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

[META 321] Make log_level configuration option dynamic #210

Closed
basepi opened this issue Oct 5, 2020 · 7 comments · Fixed by #635
Closed

[META 321] Make log_level configuration option dynamic #210

basepi opened this issue Oct 5, 2020 · 7 comments · Fixed by #635
Labels
agent-php cross APM agents Part of the alignment with the rest of Elastic APM enhancement New feature or request
Milestone

Comments

@basepi
Copy link

basepi commented Oct 5, 2020

Implementing elastic/apm#332
#134 is a prerequisite

Note that the implementation of this spec also includes adding the option to Kibana.

@SergeyKleyman SergeyKleyman added this to the Beta release milestone Oct 5, 2020
@SergeyKleyman SergeyKleyman added cross APM agents Part of the alignment with the rest of Elastic APM enhancement New feature or request labels Oct 5, 2020
@SergeyKleyman SergeyKleyman changed the title Add log_level spec Make log_level configuration option dynamic Oct 6, 2020
@SergeyKleyman SergeyKleyman removed this from the Beta release milestone Oct 6, 2020
@felixbarny
Copy link
Member

The PHP agent supports a NOTICE log level which is not one of the commonly agreed log levels. Is there a specific reason why NOTICE is required for PHP or could it be removed?

@SergeyKleyman
Copy link
Contributor

SergeyKleyman commented Nov 12, 2020

My thought process on deciding the levels was as follows:

  1. I started with the PHP standard PSR-3 which is AFAICS is direct mapping of syslog severity levels.
  2. I merged EMERGENCY, ALERT and CRITICAL into CRITICAL because I didn't see the syslog's distinction between them as relevant to the agent.
  3. I added TRACE as the most verbose level (and also the level under which the agent can log something that can be security sensitive - the agent should make best effort not to do that under less verbose levels).

So to answer your question

Is there a specific reason why NOTICE is required for PHP or could it be removed?

No, there is no specific reason - it was just inherited from syslog so it can be removed. I do see some benefits to having it - to log something that might be an issue but the code logging it doesn't have enough information to definitely say one way or the other. The way I see it, INFO is purely informational (not for drawing attention to a potential problem) and WARNING is for drawing attention to a potential problem that doesn't raise to the level of ERROR (for example WARNING for problems that might affect performance while ERROR issues might affect correctness). So NOTICE between WARNING and INFO is a "nice to have" but it's not a "must".

Do you think PHP having NOTICE level might be an issue when getting log_level from central configuration (assuming that central configuration won't have it) or is your concern more about general consistency between the agents?

@felixbarny
Copy link
Member

My only concern would just be the general cross-agent consistency. Does the PHP agent currently use the notice log level?

@SergeyKleyman
Copy link
Contributor

SergeyKleyman commented Nov 12, 2020

Yes, the agent uses NOTICE in a few place (https://github.com/elastic/apm-agent-php/search?q=ifNoticeLevelEnabled - I see that the most of it is in component tests code) but it can be easily find-and-replace-ed by let's say WARNING.

@SergeyKleyman
Copy link
Contributor

SergeyKleyman commented Nov 12, 2020

And it's going to be a breaking change for logging configuration but the agent is not GA yet so if we are going to remove NOTICE it's better to do it before GA.

@AlexanderWert AlexanderWert changed the title Make log_level configuration option dynamic [META 321] Make log_level configuration option dynamic Nov 25, 2020
@AlexanderWert AlexanderWert added this to the 8.2 milestone Jan 27, 2022
@felixbarny
Copy link
Member

@SergeyKleyman, could you link the PR that fixed this?

@SergeyKleyman
Copy link
Contributor

@felixbarny I am sorry - I was too quick in closing the issue. The PR (#635) still has some issues. You are right about missing link between PR and the issue - I will make sure to use them in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-php cross APM agents Part of the alignment with the rest of Elastic APM enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants