Skip to content

[service] Share log sampler core allocations with reflect magic #13107

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jade-guiton-dd
Copy link
Contributor

Context

PR #12617, which implemented the injection of component-identifying attributes into the zap.Logger provided to components, introduced significant additional memory use when the Collector's pipelines contain many components (#13014). This was because we would call zapcore.NewSamplerWithOptions to wrap the specialized logger core of each Collector component, which allocates half a megabyte's worth of sampling counters.

This problem was mitigated in #13015 by moving the sampling layer to a different location in the logger core hierarchy. This meant that Collector users that do not export their logs through OTLP and only use stdout-based logs no longer saw the memory increase.

Description

This PR aims to provide a better solution to this issue, by using the reflect library to clone zap's sampler core and set a new inner core, while reusing the counter allocation.

(This may also be "more correct" from a sampling point of view, ie. we only have one global instance of the counters instead of one for console logs and one for each component's OTLP-exported logs, but I'm not sure if anyone noticed the difference anyway).

Link to tracking issue

Fixes #13014

Testing

A new test was added which checks that the log counters are shared between two sampler cores with different attributes.

Copy link

codecov bot commented May 28, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 91.24%. Comparing base (8cd0191) to head (b4f5389).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...nternal/telemetry/componentattribute/logger_zap.go 78.04% 6 Missing and 3 partials ⚠️

❌ Your patch check has failed because the patch coverage (80.00%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13107      +/-   ##
==========================================
- Coverage   91.48%   91.24%   -0.24%     
==========================================
  Files         506      508       +2     
  Lines       28555    28712     +157     
==========================================
+ Hits        26123    26198      +75     
- Misses       1917     1995      +78     
- Partials      515      519       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mx-psi
Copy link
Member

mx-psi commented Jun 2, 2025

My comment from today's 1.0 meeting: I think we want to avoid the following situations:

  1. Somebody uses an incompatible zap version and the error message they get regarding this is too cryptic. Ideally the message links to somewhere where they can complain and say "hey, you broke me".
  2. We upgrade zap, this solution stops working but we don't realize

@jade-guiton-dd
Copy link
Contributor Author

@mx-psi

  1. I added a panic message when our assumptions on the sampler type fail with a link to this PR, hopefully that should work.
  2. I can't think of a way that Zap could make this stop working without either a panic on startup (eg. the Core field no longer exists) or the test failing (eg. they inline the counters inside the struct, so no more sharing).

@jade-guiton-dd jade-guiton-dd marked this pull request as ready for review June 3, 2025 09:34
@jade-guiton-dd jade-guiton-dd requested a review from a team as a code owner June 3, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collector memory increases by about ~20 MB after v0.125.0 release
2 participants