-
Notifications
You must be signed in to change notification settings - Fork 1.2k
exporter/otlploghttp: guard observ metrics with Enabled checks #7813
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
base: main
Are you sure you want to change the base?
exporter/otlploghttp: guard observ metrics with Enabled checks #7813
Conversation
|
Hey, I wante to ask you check this but what if something changes in between time start (sorry i was wrong i was using your code as refrence to understand what the changes were ) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7813 +/- ##
=======================================
- Coverage 86.1% 81.7% -4.5%
=======================================
Files 302 304 +2
Lines 22046 23247 +1201
=======================================
+ Hits 18991 18998 +7
- Misses 2675 3859 +1184
- Partials 380 390 +10
🚀 New features to boost your workflow:
|
|
Could you add tests? |
|
Hi. I can add tests. Do you expect separate cases (drop inflight/exported/duration, plus “all dropped”), or is one view-based test enough to show that when an instrument is disabled we skip it and ExportLogs().End() still records the remaining metrics? |
|
I think one test with subtests for each metric would be good. |
|
Don't trust what the LLM says. Your test needs to fail before your changes, and pass after. |
…ents are not called.
|
How would this be tested? Enabled is a performance improvement to avoid computing attributes when we know in advance the metric will be dropped. Using enabled or not shouldn't be a behavioral change, right? |
|
Might be able to demonstrate it with a benchmark when a no-op meterprovider is used? |
|
I added a unit test because another reviewer asked for tests. it basically just verifies we don’t call Add/Record when an instrument reports Enabled(ctx)==false. If you’d prefer, I can drop the unit test and add a benchmark instead since this is mainly a perf improvement. What would you rather have? |
|
Nice. I didn't think of that, and that works for me. |
|
Awesome, thanks! I’ll fix the remaining lint issues and update the PR. |
This PR updates the OTLP HTTP log exporter observability instrumentation to check instrument Enabled(ctx) before building options/attributes and recording measurements.
Behavior is unchanged when metrics are enabled; this reduces overhead when exporter self-metrics are disabled/no-op.
Part of the work tracked in #7800.