Skip to content

Avoid allocating too much memory, sampling logic is not re-applied #13015

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

Merged
merged 2 commits into from
May 12, 2025

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented May 12, 2025

Description

Avoid re-creating sampler counters every time we wrap with attributes.

Link to tracking issue

Updates #13014

Testing

Documentation

@bogdandrutu bogdandrutu requested a review from a team as a code owner May 12, 2025 03:30
@bogdandrutu bogdandrutu requested a review from codeboten May 12, 2025 03:30
@bogdandrutu bogdandrutu force-pushed the fix-13014 branch 2 times, most recently from 34a3560 to fb487ec Compare May 12, 2025 03:33
@bogdandrutu bogdandrutu added the release:blocker The issue must be resolved before cutting the next release label May 12, 2025
@bogdandrutu bogdandrutu force-pushed the fix-13014 branch 4 times, most recently from c320e21 to fc5c33f Compare May 12, 2025 03:52
Copy link

codecov bot commented May 12, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.51%. Comparing base (5800834) to head (9af9190).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
service/telemetry/logger.go 66.66% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13015      +/-   ##
==========================================
- Coverage   91.62%   91.51%   -0.11%     
==========================================
  Files         504      504              
  Lines       27888    28156     +268     
==========================================
+ Hits        25553    25768     +215     
- Misses       1844     1877      +33     
- Partials      491      511      +20     

☔ 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.

@MovieStoreGuy
Copy link
Contributor

Do you have a memory profile from this branch to compare?

@bogdandrutu
Copy link
Member Author

I manually tested that the NewSamplerWithOptions is calling only once per service compare to previously once per component.

@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented May 12, 2025

I believe this change would mean that sampling is only applied to console logs, not logs exported through OTLP. So I don't think this is the right solution.

@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented May 12, 2025

The issue is that:

  1. we need to apply sampling upstream of the otelzap core, which means wrapping a sampling core around it
  2. we need to create a new otelzap core to set new scope attributes, which requires rewrapping
  3. the counters type used by the sampling core takes up 448 KiB of memory (??)
  4. the Zap sampling cores are completely opaque, and don't offer any way of changing the inner core without recreating a brand new counters structure

I can see a few solutions:

  1. Change our approach for setting scope attributes in logs to one that doesn't require recreating anything downstream of the sampler; for example, modifying otelzap to turn scope.* fields into scope attributes. Unless we cache otel.Loggers, this would probably be expensive in other ways however...
  2. File an issue on zapcore to get a method to create a new sampler with a new inner core but with shared counters. This shouldn't be hard since the counters are already meant to be shared (the With method already does something similar).
  3. Create our own sampling core, either a functionally identical one with this sharing capability, or a simpler but more memory-efficient one. If we don't want to maintain that, it could be an alternative while waiting for the issue to be addressed.
  4. Add sampling capabilities to the OTLP pipeline separately using a different approach.

@jade-guiton-dd
Copy link
Contributor

(To try to clarify how the code functions:)

Structure created by current code Structure created by this PR
image image

Copy link
Contributor

@jade-guiton-dd jade-guiton-dd left a comment

Choose a reason for hiding this comment

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

As a quick fix for the stdout-only case, this looks good to me. I would recommend making the release note a bit more explicit, and not marking the issue as entirely fixed by this PR.

@bogdandrutu bogdandrutu added this pull request to the merge queue May 12, 2025
Merged via the queue into open-telemetry:main with commit 8991b48 May 12, 2025
55 of 57 checks passed
@bogdandrutu bogdandrutu deleted the fix-13014 branch May 12, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:blocker The issue must be resolved before cutting the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants