Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add log_level parameter to teraslice job. #3505

Merged
merged 14 commits into from
Jan 3, 2024
Merged

Conversation

busma13
Copy link
Contributor

@busma13 busma13 commented Dec 18, 2023

This PR adds the log_level parameter to a teraslice job.

  • Export loglevels from utils/src/Logger
  • Add log_level to job-components/src/job-schema
  • Within the Worker and ExecutionController constructors, use the log level from the executionContext to set the log level of context.logger before it is used to create a new child logger.
  • This child logger will be inherited by all other classes that have a logger within Worker and ExecutionController instances.

Ref: #3498

Copy link
Member

@godber godber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the default problem, and is there a way we could have implemented a test to catch that scenario.

packages/job-components/src/job-schemas.ts Outdated Show resolved Hide resolved
@busma13 busma13 force-pushed the set-log-level-from-job branch from 37aa7e8 to 21b423e Compare December 21, 2023 15:40
@busma13 busma13 marked this pull request as ready for review January 2, 2024 19:30
@busma13 busma13 requested a review from godber January 2, 2024 19:44
Copy link
Member

@godber godber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump sub package versions and teraslice version to 0.90.0 (both top level and subpackage) and consider the inline suggestion

Comment on lines 217 to 219
if (typeof level !== 'string') {
throw new Error('must be of type string');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the check against logLevels I think this test for string might be redundant and less useful. Imaging a user provides a number 5, then they will be told to use a string. Then they put in the string noisy, then it finally tells them the acceptible set of strings. Maybe the next check should test for string and includes(level).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I will combine the two checks and just have one error message.

@godber
Copy link
Member

godber commented Jan 3, 2024

There's not a whole lot of code here, but since it cross cuts packages and schemas and things we might ask for @jsnoble's input.

@busma13 busma13 force-pushed the set-log-level-from-job branch 2 times, most recently from 27772eb to 5b07d02 Compare January 3, 2024 14:58
@busma13 busma13 requested a review from jsnoble January 3, 2024 15:35
Copy link
Member

@jsnoble jsnoble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @busma13 and asked to put in some comments and double check the log levels are changing when logs are sent to the file, also to put the type log_level in the types package

@busma13
Copy link
Contributor Author

busma13 commented Jan 3, 2024

I added an explanatory comment on each of the constructors that overwrites the log level. I also confirmed that the log_level indicated in the job is written to a log file (all log streams are overwritten with new log level).

@busma13 busma13 requested review from jsnoble and godber January 3, 2024 19:28
@busma13 busma13 force-pushed the set-log-level-from-job branch from 1674bc7 to ac537de Compare January 3, 2024 21:35
@busma13 busma13 force-pushed the set-log-level-from-job branch from ac537de to 245b7e7 Compare January 3, 2024 21:43
@godber godber merged commit 0f0adb2 into master Jan 3, 2024
39 checks passed
@godber godber deleted the set-log-level-from-job branch January 3, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants