Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[telemetrySettting] Create sampled Logger #8134
[telemetrySettting] Create sampled Logger #8134
Changes from 5 commits
4394496
3192d88
39f95e4
5295e11
9534fbb
56b71f3
5aec726
5d2cdeb
7ab0227
9778c88
b6ac5ac
6c2051a
f53561a
310b747
9540c55
20536a1
0b60ef8
0add644
37fcef7
70f05d8
a5d054b
868041f
8a321ac
0cb309c
bc250fb
7180926
15b3b42
409f6ff
66fa58e
4b84a00
fcd2b83
ce3776a
bce885d
f9ca8a7
8c3cddd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codeboten, do you know why we create a sampled logger here? Shouldn't loggers be created based on the telemetry configuration alone? Instead of this PR, I would consider removing this sampled logger altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added in #2020 by @tigrannajaryan. The PR description says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, and shouldn't this be a collector-wide setting, instead of something exclusively to this helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was meant to be specific to this helper to avoid flooding the logs in the case where all requests fail, and you log once per request. The contributing guidelines say
I guess at the time this was written metrics were not really available and thus we went with sampled logs.
(Not sure if this is a good reason today to keep this, just trying to reconstruct the history of this to make a better decision)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: I'm sure this was a good decision by then, not so sure the next iteration should be to expose a setting for this instead of using a metric and/or exposing log sampling as part of the collector's telemetry config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a possible future option is to supply 2 loggers to every component: one that has no sampling (outputs 100% of logs) and another that is sampled. Components will use the no-sampled logger for everything that is critical to be printed and is known to be non-repeating so it can't flood logs (e.g. startup and shutdown messages. The messages that can repeat for every data item or request will use the sampled logger. The sampled logger can derive its sampling ratio from a central setting (somewhere in the
telemetry
config section).I do not think merely passing a single sampled logger to every component to be used for every message is good enough since it can result in critical one-off messages being lost.
On a side note: the memory usage by sampled logger that this PR attempts to address is puzzling. Maybe try to figure out why and reduce that instead of disabling sampling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tigrannajaryan answering the side note. As you can see on the graph https://user-images.githubusercontent.com/5960625/255850931-f9b3e891-2695-4aac-b8d6-9d60cb0dfb9d.png the memory problem is coming from the logger sampling. We also tried modifying the configuration of
NewSamplerWithOptions
But it was still a high memory consumption.
So, it would be great to disable the logger sampler somehow from the
telemetry
config (in a long-term solution) or from theexporterHelper
config (short-term solution)Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @tigrannajaryan. This doesn't seem like a good option to expose in the exporters' configuration interface.
@antonjim-te can you please try initializing one sampled logger instance per service and see if that helps with memory utilization? It can be created in
service.New
function and passed as another field ofTelemetrySettings
to all components. If it doesn't help with memory utilization, we can still expose a similar configuration for the sampled logger in theservice::telemetry
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dmitryax ,
Thank you for your suggestion. I will work on that in a separate branch, test it and if it solves the problem I will bring those changes to into this PR.
I will create a ticket in our internal process and add it to our next sprint.
I have a good feeling about this suggestion because it will just create one logger sampled for all the exporters instead of creating one per
exporterHelper
.Thank you, team. I will keep you updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I have followed the suggestion from @dmitryax and created an extra sampled logger in the
TelemetrySettings
and used it when necessary in our case in theQueuedRetrySender
The new approach also solved the memory incident.
Please take a look at the new PR changes.
Thank you in advance.