Skip to content

Fix Logger Severity String Parse #9629

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix Logger Severity String Parse #9629

wants to merge 1 commit into from

Conversation

sreyhani
Copy link

parsed severity string is cached as an optional variable. so parsing would be called only once for each logger.

fixes #9534

@sreyhani
Fix Logger Severity String Parse
3c0d010 
parsed severity string is cached as an optional variable. so parsing would be called only once for each logger.

refs: Icinga#9534
@cla-bot
Copy link

cla-bot bot commented Jan 15, 2023

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @sreyhani

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@icinga-probot icinga-probot bot added area/log Logging related core/quality Improve code, libraries, algorithms, inline docs good first issue Good for newcomers labels Jan 15, 2023
@sreyhani
Copy link
Author

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @sreyhani

signed the CLA

@sreyhani
Copy link
Author

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Jan 16, 2023

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @sreyhani

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

krishnacx
krishnacx reviewed Jan 16, 2023
View reviewed changes
lib/base/logger.cpp
@@ -202,7 +211,7 @@ bool Logger::IsTimestampEnabled()
void Logger::SetSeverity(const String& value, bool suppress_events, const Value& cookie)
Copy link

Choose a reason for hiding this comment

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

Can we add function description as well ?

@bobapple
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Jan 19, 2023
@Al2Klimov
Copy link
Member

Is the function called that often?

@sreyhani
Copy link
Author

sreyhani commented Jan 19, 2023 •
edited
Loading

Is the function called that often?

it is called for each Log message * number of loggers

I don't know if that's really high number or not. just wanted to fix the related issue

@Al2Klimov
Copy link
Member

At the end of the day you only changed GetMinSeverity which is only called here in UpdateMinLogSeverity which is only called in

  1. Logger::Start
  2. Logger::Stop

@sreyhani
Copy link
Author

At the end of the day you only changed GetMinSeverity which is only called here in UpdateMinLogSeverity which is only called in

  1. Logger::Start
  2. Logger::Stop

GetMinSeverity is also called in ~Log here

@Al2Klimov
Copy link
Member

Bildschirm­foto 2023-01-20 um 10 11 33

GH search @ its best.

@Al2Klimov Al2Klimov self-requested a review January 20, 2023 09:15
Al2Klimov
Al2Klimov reviewed Jan 20, 2023
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Anyway I prefer #9534 (comment) and would wait whether that suggestion's author will do that.

lib/base/logger.cpp
@@ -202,7 +211,7 @@ bool Logger::IsTimestampEnabled()
void Logger::SetSeverity(const String& value, bool suppress_events, const Value& cookie)
{
ObjectImpl<Logger>::SetSeverity(value, suppress_events, cookie);

min_severity.emplace(StringToSeverity(value));
Copy link
Member

Choose a reason for hiding this comment

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

What happens...

lib/base/logger.cpp

return ls;
} catch (const std::exception &) { /* use the default level */ }
min_severity.emplace(ls);
Copy link
Member

Choose a reason for hiding this comment

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

... in case of an exception?

@Al2Klimov Al2Klimov added the stalled Blocked or not relevant yet label Feb 1, 2023
@Al2Klimov Al2Klimov removed the good first issue Good for newcomers label Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Al2Klimov Al2Klimov Al2Klimov left review comments

@krishnacx krishnacx krishnacx left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/log Logging related cla/signed core/quality Improve code, libraries, algorithms, inline docs stalled Blocked or not relevant yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Logger severity string parsed for each log message
4 participants
@sreyhani @bobapple @Al2Klimov @krishnacx