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 simple scope configuration to Tracer, Meter, Logger #3877

Merged
merged 9 commits into from Apr 8, 2024

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Feb 15, 2024

Fixes #3867.

This introduces the concept of scope-level configuration for Tracer, Meter, and Logger. Notes:

  • TracerProvider, MeterProvider, and LoggerProvider have a scope config provider function, which accepts an InstrumentationScope and returns the relevant scope config. This function is invoked when a Tracer, Meter, Logger is created, and the resulting scope config dictates the behavior of resulting Tracers, Meters, Loggers. Computing the config on scope creation avoids doing extra work on the hot path.
  • If TracerProvider, MeterProvider, LoggerProvider supports updating configuration, the function can be updated, at which point the scope config of outstanding Tracer, Meter, Logger will be re-computed. This accommodates emerging dynamic config use cases.
  • A function as the mechanism to determine relevant scope config to maximize flexibility. However, implementations are encourages to provide syntactic sugar / helpers for accommodating popular use cases: select one or more scopes by name or with pattern matching, disable one or more selected scopes, disable all scopes by default and enable one or more selective scopes.
  • Scope config is simple at the moment, consisting of on the ability to enable / disable a particular Tracer, Meter, Logger. When a disabled, use the respective noop Tracer, Meter, Logger.
  • Scope config parameters are expected to evolve over time. When it makes sense, there should be consistency across signals.
  • Java prototype available for this proposal: Prototype disabling scope across traces, metrics, and logs opentelemetry-java#6201

Leaving as a draft for now because its not ready to merge, but I am actively soliciting feedback. If / when we agree on direction, I'll go back and add the appropriate experimental indications.

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 23, 2024
@jack-berg jack-berg removed the Stale label Feb 23, 2024
@jack-berg jack-berg marked this pull request as ready for review February 25, 2024 15:23
@jack-berg jack-berg requested review from a team as code owners February 25, 2024 15:23
@tsloughter
Copy link
Member

Glad to see this (Erlang/Elixir actually still has an undocumented ability to disable Tracer's per-scope that exists I believe because of a very early spec iteration) but I'm not sure it should be a "Provider". If it is a function and not a stateful "object" it doesn't sound like a Provider to me.

The TracerProvider already feels like the TracerConfigProvider while the per-scope configuration is a part of its configuration.

Random other thoughts:

I recall talk of things like SamplerProviders to make it possible to configure per-scope, but I like the idea of making tracer configuration a per-scope thing.

I believe there was also the idea that if you wanted different configurations you used different TracerProviders, removing the need for per-scope configuration, but there was nothing about configuring specific scopes to use specific providers.

@jack-berg
Copy link
Member Author

but I'm not sure it should be a "Provider". If it is a function and not a stateful "object" it doesn't sound like a Provider to me

Any recommendations for alternative names? Other options I considered:

  • TraceConfigProvider / MeterConfigProvider / LoggerConfigProvider: Name it as if you were naming an interface which just has a single method. Use the "Provider" suffix, which is familiar / intuitive. I.e. its clear that this thing "provides" the noun prefix. But may be confusing to see multiple things classes of things called "providers".
  • TracerConfigurer / MeterConfigurer / LoggerConfigurer: Name it as if you were naming an interface which just has a single method. Just "configurer" suffix to differentiate from TracerProvider / MeterProvider / LoggerProvider. Configurer is a bit of a mouthful and doesn't show up too often in software design patterns.
  • computeTracerConfig / computeMeterConfig / computeLoggerConfig: Name it like you would name a function. The "compute" prefix helps convey that some compute may be involved and isn't to be called on the hot path.
  • getTracerConfig / getMeterConfig / getLoggerConfig: Name it like you would name a function. The "get" prefix is shorter than "compute", but may cause confusion if users look for a corresponding "set" method, which doesn't exist.
  • tracerConfig / meterConfig / loggerConfig: Name it like you would name a function. Use the naming style where the function is named for the noun it produces.

I recall talk of things like SamplerProviders to make it possible to configure per-scope, but I like the idea of making tracer configuration a per-scope thing.

I think scope config is the natural way to accommodate that type of thing. Easy to imagine evolving TracerConfig with another option for the specific sampler that should be used with that tracer.

I believe there was also the idea that if you wanted different configurations you used different TracerProviders, removing the need for per-scope configuration, but there was nothing about configuring specific scopes to use specific providers.

Yes, no way to specify that a particular scope should use a specific provider. This is especially true in auto instrumentation scenarios like the otel java agent. Even if it were possible, it would be pretty awful to have a different batch span processor and span exporter for each scope.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

The name TracerConfigProvider can carry the following implicit semantics, comparing it with existing providers:

  • There is a TracerConfigProvider singleton somewhere
  • Applications are to set this singleton
  • TracerProvider are to use this singleton

In my understanding, this is not the case, and TracerConfigProvider will be an additional property of a TracerProvider.

In this case, I would rename it to TracerConfigurator (changed), which produces TracerConfig (unchanged)

@jack-berg
Copy link
Member Author

In this case, I would rename it to TracerConfigurator (changed), which produces TracerConfig (unchanged)

I have no problem with TracerConfigurator. What do other folks think?

@jack-berg
Copy link
Member Author

Based on feedback, I pushed a commit which renamed to {Tracer|Meter|Logger}Configurator.

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 23, 2024
@jack-berg jack-berg removed the Stale label Mar 25, 2024
@jack-berg
Copy link
Member Author

@open-telemetry/specs-approvers please take a look. This solves an important use case, has decent level support as indicates in the comments of #3877, and fixes a conceptual gap in the current lack of sdk tools around scope. Merging this would be good for SDKs.

specification/logs/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/logs/sdk.md Outdated Show resolved Hide resolved
specification/logs/sdk.md Show resolved Hide resolved
specification/logs/sdk.md Show resolved Hide resolved
@MadVikingGod
Copy link
Contributor

Has anyone checked if dynamic config is compatible with how Tracers/MeterProviders/Loggers are used currently?

The current recommendation for go is to get a Tracer, etc., once and reuse it often. An example is the net/http middleware will use the tracer, and by extension the current scope config, when the middleware is created and never again. While this PR would enable a user to disable/enable scopes at the start of a program, it wouldn't enable dynamic updating of config. I haven't audited the rest of go instrumentation, but I think this is the common pattern, and wonder what other languages do in these cases.

@jack-berg
Copy link
Member Author

Has anyone checked if dynamic config is compatible with how Tracers/MeterProviders/Loggers are used currently?

As mentioned here, this PR doesn't introduce dynamic config, but it is designed to be performant in a future world where dynamic config is better understood and supported.

The current recommendation for go is to get a Tracer, etc., once and reuse it often. An example is the net/http middleware will use the tracer, and by extension the current scope config, when the middleware is created and never again. While this PR would enable a user to disable/enable scopes at the start of a program, it wouldn't enable dynamic updating of config. I haven't audited the rest of go instrumentation, but I think this is the common pattern, and wonder what other languages do in these cases.

We need to explore this more, but doing so shouldn't block this PR. Currently, there is no API for instrumentation to determine if a particular Tracer / Meter / Logger is enabled. This means that even with this PR, instrumentation can't be optimized to not collect the data needed to produce telemetry based on SDK config. But as noted in #3917 and this comment, being able to check if Tracer / Meter / Logger is enabled is important.

Let's add that in a followup. Supposing we do, then instrumentations will need to decide what their strategy is for checking if Tracer, Meter, Logger is enabled:

  • They could skip checking it ever. This causes instrumentation to pay a performance penalty even tho the user doesn't want to use the telemetry.
  • They could check once at app start. This is simple, but doesn't accommodate dynamic config scenarios, which we know will be important at some point in the future.
  • They could check back on each operation or periodically. This accommodates dynamic config scenarios, but makes instrumentation more complex. Also, how does this work in auto instrumentation scenarios like the otel java agent which installs instrumentation by performing bytecode manipulation at app start? Despite these challenges, I think this is the best place to land long term.

But this is a very useful feature even without the followup to add API surface area to check if Tracer / Meter / Logger is enabled.

Copy link

github-actions bot commented Apr 2, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 2, 2024
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Show resolved Hide resolved
@jack-berg jack-berg merged commit 223c907 into open-telemetry:main Apr 8, 2024
7 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.

Turn off spans from a scope while retaining downstream spans and without breaking traces