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

Add APIs to determine if tracer, logger, instruments are enabled #6502

Merged
merged 6 commits into from
Jul 3, 2024

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Jun 4, 2024

Followup to #6497.

Spec PRs open-telemetry/opentelemetry-specification#4063 and open-telemetry/opentelemetry-specification#4020 extended the SDK scope config options with corresponding APIs, which can be called to avoid unnecessary computation recording telemetry when the SDK isn't going to do anything with it.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 71.69811% with 15 lines in your changes missing coverage. Please review.

Project coverage is 90.59%. Comparing base (9bfb81b) to head (23a008b).

Files Patch % Lines
...ics/internal/state/MultiWritableMetricStorage.java 20.00% 3 Missing and 1 partial ⚠️
...entelemetry/api/incubator/logs/ExtendedLogger.java 0.00% 1 Missing ⚠️
...y/api/incubator/metrics/ExtendedDoubleCounter.java 0.00% 1 Missing ⚠️
...try/api/incubator/metrics/ExtendedDoubleGauge.java 0.00% 1 Missing ⚠️
...api/incubator/metrics/ExtendedDoubleHistogram.java 0.00% 1 Missing ⚠️
...incubator/metrics/ExtendedDoubleUpDownCounter.java 0.00% 1 Missing ⚠️
...try/api/incubator/metrics/ExtendedLongCounter.java 0.00% 1 Missing ⚠️
...metry/api/incubator/metrics/ExtendedLongGauge.java 0.00% 1 Missing ⚠️
...y/api/incubator/metrics/ExtendedLongHistogram.java 0.00% 1 Missing ⚠️
...i/incubator/metrics/ExtendedLongUpDownCounter.java 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6502      +/-   ##
============================================
- Coverage     90.63%   90.59%   -0.05%     
- Complexity     6228     6255      +27     
============================================
  Files           679      689      +10     
  Lines         18661    18698      +37     
  Branches       1842     1843       +1     
============================================
+ Hits          16914    16940      +26     
- Misses         1189     1201      +12     
+ Partials        558      557       -1     

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

@jack-berg jack-berg marked this pull request as ready for review June 6, 2024 14:47
@jack-berg jack-berg requested a review from a team June 6, 2024 14:47
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

it looks like there's a mix of boolean isEnabled() and boolean enabled()

@jack-berg
Copy link
Member Author

Good call! Resolved.

@jkwatson
Copy link
Contributor

Is the title of this PR correct? Is this just a stepping stone toward being able to have API disabled via some sort of configuration, per scope?

@jack-berg
Copy link
Member Author

There's already (experimental) SDK config to disable scopes. This adds (experimental) API surface area which allows instrumentation to inspect whether a scope is disabled, and if yes, avoid unnecessary computation in recording instrumentation.

The idea of having easier SDK configuration for disabling these (i.e. file config, or something else) is independent and not addressed by this.

@jkwatson
Copy link
Contributor

There's already (experimental) SDK config to disable scopes. This adds (experimental) API surface area which allows instrumentation to inspect whether a scope is disabled, and if yes, avoid unnecessary computation in recording instrumentation.

The idea of having easier SDK configuration for disabling these (i.e. file config, or something else) is independent and not addressed by this.

So...is the title of this PR incorrect, then?

@jack-berg
Copy link
Member Author

So...is the title of this PR incorrect, then?

I'm not following.. This PR does add an API to check if a particular scope is enabled.

@jkwatson
Copy link
Contributor

jkwatson commented Jul 2, 2024

So...is the title of this PR incorrect, then?

I'm not following.. This PR does add an API to check if a particular scope is enabled.

So...is the title of this PR incorrect, then?

I'm not following.. This PR does add an API to check if a particular scope is enabled.

It might do that indirectly; what it does is add APIs to see if a given tracer/instrument/logger is enabled. Those can be scope associated, but don't have to be. /shrug

@jack-berg
Copy link
Member Author

It might do that indirectly; what it does is add APIs to see if a given tracer/instrument/logger is enabled. Those can be scope associated, but don't have to be. /shrug

Ahh I see. We're talking past each other on language. I use the phrase "scope" to refer to the concept of tracer / logger / meter collectively, since tracer / logger / meter are the signal-specific terms for scope. Admittedly, its a bit imprecise. I'll update the PR title.

@jack-berg jack-berg changed the title Scope enabled api Add APIs to determine if tracer, logger, instruments are enabled Jul 2, 2024
@jkwatson
Copy link
Contributor

jkwatson commented Jul 2, 2024

It might do that indirectly; what it does is add APIs to see if a given tracer/instrument/logger is enabled. Those can be scope associated, but don't have to be. /shrug

Ahh I see. We're talking past each other on language. I use the phrase "scope" to refer to the concept of tracer / logger / meter collectively, since tracer / logger / meter are the signal-specific terms for scope. Admittedly, its a bit imprecise. I'll update the PR title.

Yep! I like the new PR title better, as I think it will cause less confusion for everybody in the future. Also, "Scope" is used in the Context APIs, which also could lead to confusion.

What API will actually allow users to enable/disable these things, BTW?

@jack-berg
Copy link
Member Author

What API will actually allow users to enable/disable these things, BTW?

An experimental API for this already exists. See ScopeConfiguratorTest, LoggerConfigTest, MeterConfigTest, TracerConfigTest for usage details.

Ergonomics will improve once the API is stabilized.

@jack-berg jack-berg merged commit 1f7d6a5 into open-telemetry:main Jul 3, 2024
17 of 18 checks passed
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.

3 participants