Skip to content

[unstable] Add log level override #452

Merged
devendra-lohar merged 5 commits intomasterfrom
unstable/add-log-level-override
Jul 7, 2025
Merged

[unstable] Add log level override #452
devendra-lohar merged 5 commits intomasterfrom
unstable/add-log-level-override

Conversation

@devendra-lohar
Copy link
Copy Markdown
Contributor

@devendra-lohar devendra-lohar commented Jul 2, 2025

This PR adds support to override the log level defined in the application config from the cli.

@devendra-lohar devendra-lohar changed the title [unstable] Add log level override changes [unstable] Add log level override Jul 2, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 2, 2025

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.96%. Comparing base (308828c) to head (50512b0).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
cognite/extractorutils/unstable/core/base.py 83.33% 2 Missing ⚠️
cognite/extractorutils/unstable/core/runtime.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #452      +/-   ##
==========================================
+ Coverage   78.02%   78.96%   +0.93%     
==========================================
  Files          42       42              
  Lines        3627     3636       +9     
==========================================
+ Hits         2830     2871      +41     
+ Misses        797      765      -32     
Files with missing lines Coverage Δ
cognite/extractorutils/unstable/core/runtime.py 47.36% <0.00%> (-0.32%) ⬇️
cognite/extractorutils/unstable/core/base.py 77.09% <83.33%> (+18.61%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@devendra-lohar devendra-lohar marked this pull request as ready for review July 2, 2025 11:52
@devendra-lohar devendra-lohar requested a review from a team as a code owner July 2, 2025 11:52
# Use the override level if provided
level_to_set = _resolve_log_level(self.log_level_override)
min_level = level_to_set
max_level = level_to_set
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is min and max level for the log should be same in case log level override? Or is it configurable too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think when we use the --log-level override from the command line, the intended behavior should be to force all log handlers to a single, specific level unlike the config where there can be multiple log-handlers and hence multiple log-levels.

Copy link
Copy Markdown
Contributor

@toondaey toondaey left a comment

Choose a reason for hiding this comment

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

Nice work. But I believe there's another _setup_logging in runtime.py that you may need to override.

@devendra-lohar
Copy link
Copy Markdown
Contributor Author

Nice work. But I believe there's another _setup_logging in runtime.py that you may need to override.

The _setup_logging method in the Runtime class configures a very basic logger for the runtime process itself and is hardcoded to the INFO level to ensure initial startup messages are always visible.

My initial thought was to keep them separate to guarantee that critical startup messages are always visible. Let me know if I should adjust if a unified approach is preferred.

@devendra-lohar devendra-lohar requested a review from toondaey July 3, 2025 09:15
@toondaey
Copy link
Copy Markdown
Contributor

toondaey commented Jul 3, 2025

Nice work. But I believe there's another _setup_logging in runtime.py that you may need to override.

The _setup_logging method in the Runtime class configures a very basic logger for the runtime process itself and is hardcoded to the INFO level to ensure initial startup messages are always visible.

My initial thought was to keep them separate to guarantee that critical startup messages are always visible. Let me know if I should adjust if a unified approach is preferred.

You're right. The ticket does also say just log level in the extractor config.

toondaey
toondaey previously approved these changes Jul 3, 2025
@devendra-lohar devendra-lohar added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Jul 3, 2025
@toondaey
Copy link
Copy Markdown
Contributor

toondaey commented Jul 4, 2025

I just realized that we need an entry in the changelog for this.

Copy link
Copy Markdown
Contributor

@toondaey toondaey left a comment

Choose a reason for hiding this comment

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

Hopefully, no conflicts.

@rsjr rsjr self-assigned this Jul 4, 2025
@rsjr rsjr added the risk-review-ongoing Risk review is in progress label Jul 4, 2025
Copy link
Copy Markdown
Contributor

@rsjr rsjr left a comment

Choose a reason for hiding this comment

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

risk review ok

@rsjr rsjr added waiting-for-team Waiting for the submitter or reviewer of the PR to take an action and removed waiting-for-risk-review Waiting for a member of the risk review team to take an action labels Jul 4, 2025
Copy link
Copy Markdown
Contributor

@Hmnt39 Hmnt39 left a comment

Choose a reason for hiding this comment

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

Looks good

@devendra-lohar devendra-lohar requested a review from rsjr July 7, 2025 05:40
@devendra-lohar devendra-lohar added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Jul 7, 2025
Copy link
Copy Markdown
Contributor

@rsjr rsjr left a comment

Choose a reason for hiding this comment

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

risk review ok

@devendra-lohar devendra-lohar merged commit 6a0a3f1 into master Jul 7, 2025
6 checks passed
@devendra-lohar devendra-lohar deleted the unstable/add-log-level-override branch July 7, 2025 07:03
toondaey added a commit that referenced this pull request Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-review-ongoing Risk review is in progress waiting-for-risk-review Waiting for a member of the risk review team to take an action waiting-for-team Waiting for the submitter or reviewer of the PR to take an action

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants