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

Align log_level option + central config #321

Closed
felixbarny opened this issue Aug 14, 2020 · 13 comments · Fixed by #332
Closed

Align log_level option + central config #321

felixbarny opened this issue Aug 14, 2020 · 13 comments · Fixed by #332

Comments

@felixbarny
Copy link
Member

felixbarny commented Aug 14, 2020

Currently, all agents except the Python agent support the log_level option.

We'd like to add this to central config to make it easier to increase the log level on-the-fly to troubleshoot issues without having to restart the application.

Open Questions

  • Does exposing a log_level option make sense for Python? Currently, the log level is configured via the framework's log config. Would adding an Elastic APM specific log_level option interfere with that? How do other agents, such as the .NET agent handle this that can also be configured via the framework's logging config?
  • Is it feasible to make this option dynamic for all agents?
  • Should we align on values?
    • Would it be strange for .NET or Python user, for example, to use different log_levels in central config as opposed to the ones used in the framework's logging config?
    • Do we need to enable users to set the log_level across apps monitored by different agents?

Status

Spec: #332

Agent Milestone Implementation issue Done Added to Kibana
dotnet elastic/apm-agent-dotnet#970
go elastic/apm-agent-go#829
java 7.10 elastic/apm-agent-java#1426
nodejs 7.11 elastic/apm-agent-nodejs#1826
php elastic/apm-agent-php#210
python 7.10 elastic/apm-agent-python#941
ruby 7.11 elastic/apm-agent-ruby#878
rum-js elastic/apm-agent-rum-js#910
@basepi
Copy link
Contributor

basepi commented Aug 14, 2020

Does exposing a log_level option make sense for Python?

Yes, I think it does. The current way we recommend configuring logging is correct, but a little bit user-hostile. Having an easy way to get our agent logging more verbosely is probably a good thing to have, and aligns us with the other agents.

Is it feasible to make this option dynamic for all agents?

I think we could make it dynamic for python but it would require a re-work of our configuration handling. We would need to add the ability to tie function calls to configuration changes in our remote config implementation, because we would have to execute code to change the log level. Should be doable, but it's a non-trivial amount of work.

Should we align on values?

This is where I'm most torn. I'm currently leaning "no" because I think it's asking for user confusion.

@felixbarny
Copy link
Member Author

@sqren would it be feasible for the central config UI to have a different set of options in a dropdown for each agent?

@mikker
Copy link
Contributor

mikker commented Aug 18, 2020

All is good for Ruby 👍

@cauemarcondes
Copy link

would it be feasible for the central config UI to have a different set of options in a dropdown for each agent?

@felixbarny yes, we can have a dropdown field with a different set of options for each agent.

@dgieselaar
Copy link
Member

@felixbarny should this be an option that is available for all agents? ie, will it be available when we want to create a configuration for all services? In that case, how do we decide what option values should be shown?

As a reference, this is how it currently looks when you want to create a configuration for all services:

image

@felixbarny
Copy link
Member Author

When we go with different values for each agent, I guess it's not feasible to show the log_level when selecting all agents.

Are there plans to create configurations for all Ruby agents, for example, or based on other metadata like availability zones or global labels?

@dgieselaar
Copy link
Member

@felixbarny I'm not aware of any plans.

@felixbarny
Copy link
Member Author

felixbarny commented Aug 19, 2020

I just realized that log_level is already in central config and available for the Ruby and .NET agents:

https://github.com/elastic/kibana/blob/0efe286fad9f1b008478b43549c186ad281f3251/x-pack/plugins/apm/common/agent_configuration/setting_definitions/general_settings.ts#L91-L103

Currently, the central config UI has a just plain text input field.

There are two sensible ways forward

Option 1: Contintue with per-agent values
We currently don't align but still have it in central config so it's fine to add other agents with their specific log levels.
If we'd want to align at some point in the future we can do it that in a backwards compatible way.

Option 2: Take the opportunity to align
Let's take adding all agents to central config as an opportunity to align on the values.
It will be easier and less confusing for our users to only have one set of values.
I had a look at all of the possible values for our agents and there's already a lot of overlap.
Log levels are surprisingly universal so there's not a lot of potential for confusion.

If we go down this route I'd propose consolidating on these values: trace, debug, info, warning, error, critical, off
To remain backwards compatibility, agents would still accept their "native" log levels. But in central config, there's a dropdown with these values that are consistent for each agent.
Some agents, such as the Go agent, only use a subset of these values but I think that's fine. If an agent doesn't have trace, it would map it to the same level as debug. If the underlying logging framework doesn't support critical, agents can treat that as a synonym for error.

The off level would be a switch to completely turn off logging.


Personally, I'm slightly in favor of option 2. It seems like it's not a lot of effort™ and there's not a lot of potential for confusion. Not aligning feels a bit like a missed opportunity to make it easier for the user.

@dgieselaar
Copy link
Member

+1 on aligning. It means that we can offer a better UX in the agent configuration wizard.

@basepi
Copy link
Contributor

basepi commented Aug 19, 2020

Aligning should be fine. That list is very convenient for the python agent, it aligns very well with what's available by default.

@SergeyKleyman
Copy link
Contributor

+1 on aligning.

One minor comment regarding the naming of the levels: I would recommend warning instead of warn since warning is one letter shorter than critical anyway.

@felixbarny
Copy link
Member Author

Superseded by #332

@felixbarny felixbarny linked a pull request Aug 26, 2020 that will close this issue
@AlexanderWert
Copy link
Member

Reopening this issue to track the meta status of this feature

@AlexanderWert AlexanderWert reopened this Nov 25, 2020
@AlexanderWert AlexanderWert added this to the 7.11 milestone Nov 25, 2020
@AlexanderWert AlexanderWert removed this from the 7.11 milestone Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants