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

[exporter/logging] Build exporter's logger from service's logger #5677

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Jul 12, 2022

Description:

Build the logger on the logging exporter's logger from the one on TelemetrySettings, so that the logger honors the configuration settings on telemetry::logs and the configuration options on CollectorSettings.LoggingOptions.

Depends on #5678

Link to tracking Issue: Fixes #5652

Documentation: Documented the feature gate.

@mx-psi mx-psi force-pushed the mx-psi/logging-exporter-core branch from 69c78c3 to 16b18ad Compare July 12, 2022 15:31
@mx-psi mx-psi marked this pull request as ready for review July 12, 2022 15:31
@mx-psi mx-psi requested review from a team and tigrannajaryan July 12, 2022 15:31
@mx-psi

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #5677 (246c18b) into main (a7c047b) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5677      +/-   ##
==========================================
+ Coverage   91.44%   91.53%   +0.09%     
==========================================
  Files         192      192              
  Lines       11416    11398      -18     
==========================================
- Hits        10439    10433       -6     
+ Misses        778      770       -8     
+ Partials      199      195       -4     
Impacted Files Coverage Δ
exporter/loggingexporter/factory.go 100.00% <100.00%> (+22.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7c047b...246c18b. Read the comment docs.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Arguably, the current behavior (not this PR's behavior) is desirable. When I am troubleshooting I often add a logging exporter and set it to debug level. I don't also want to enable debug level for everything else, it will be just too much noise in the logs.

If we feel there is inconsistency because logging exporter's debug level does not honour the main level setting, I suggest we do something else instead: the "loglevel" setting of the logging exporter should control whether the detailed output is generated, however that detailed output should be generated using info level, not debug level. So, essentially "loglevel" setting will control the behavior of the logging exporter, not the behavior of the logging exporter's logger. All output from logging exporter will happen at the info level. Once that change is made you can then pass the service's logger to the logging exporter to use.

@mx-psi
Copy link
Member Author

mx-psi commented Jul 12, 2022

@tigrannajaryan Right, this was the first alternative mentioned in #5677 (comment). It feels a bit weird to me, but I understand your point about wanting to filter the logs.

PTAL at #5678 which implements the suggested change. I will update this PR once that one is merged, marking as draft until then

@mx-psi mx-psi marked this pull request as draft July 12, 2022 16:37
@mx-psi mx-psi force-pushed the mx-psi/logging-exporter-core branch from 91c1643 to 9f78d01 Compare July 13, 2022 08:44
@mx-psi mx-psi changed the title [exporter/logging] Add feature gate to build exporter from service's logger [exporter/logging] Build exporter's logger from service's logger Jul 13, 2022
@mx-psi
Copy link
Member Author

mx-psi commented Jul 13, 2022

Rewritten to not have breaking changes; depends on #5678

@mx-psi mx-psi force-pushed the mx-psi/logging-exporter-core branch from 9f78d01 to 066c567 Compare July 13, 2022 14:39
@mx-psi mx-psi marked this pull request as ready for review July 13, 2022 14:40
@mx-psi
Copy link
Member Author

mx-psi commented Jul 13, 2022

@tigrannajaryan this is ready for another look, I updated the PR description; this no longer introduces any breaking changes

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

LGTM

@mx-psi mx-psi added the ready-to-merge Code review completed; ready to merge by maintainers label Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[exporter/logging] Logging exporter logger does not honor telemetry logs configuration
4 participants