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 experimental updateTracerConfigurations for dynamically updateable on/off instrumentation #6899

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

Conversation

jackshirazi
Copy link
Contributor

This enables dynamic config - it's all that is needed at the experimental level. For the complete solution it would be a TracerProvider.updateTracerConfigurations() interface method addition.

A usage example is https://github.com/elastic/elastic-otel-java/blob/main/custom/src/main/java/co/elastic/otel/config/DynamicInstrumentation.java

Essentially, the configuration is set with an updatable ScopeConfigurator (see below fragment), then that can be updated at any time with eg GlobalOpenTelemetry.getTracerProvider().updateTracerConfigurations()

  public void customize(AutoConfigurationCustomizer autoConfiguration) {
    autoConfiguration.addTracerProviderCustomizer(
        (providerBuilder, properties) -> {
          providerBuilder.setTracerConfigurator(
              UpdatableConfigurator.INSTANCE);
          return providerBuilder;
        });
  }

  public static class UpdatableConfigurator implements ScopeConfigurator<TracerConfig> {
    public static final UpdatableConfigurator INSTANCE = new UpdatableConfigurator();
    private final ConcurrentMap<String, TracerConfig> map = new ConcurrentHashMap<>();

    private UpdatableConfigurator() {}

    @Override
    public TracerConfig apply(InstrumentationScopeInfo scopeInfo) {
      return map.getOrDefault(scopeInfo.getName(), TracerConfig.defaultConfig());
    }

    public void put(InstrumentationScopeInfo scope, TracerConfig tracerConfig) {
      map.put(scope.getName(), tracerConfig);
    }
  }

I haven't added implementations for DefaultTracerProvider and ExtendedDefaultTracerProvider nor tests, because I feel this is fine for the experimental stage, but those could be added easily

@jackshirazi jackshirazi requested a review from a team as a code owner November 21, 2024 16:30
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 13.33333% with 13 lines in your changes missing coverage. Please review.

Project coverage is 90.25%. Comparing base (7d74ada) to head (dde5f7e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...in/java/io/opentelemetry/sdk/OpenTelemetrySdk.java 0.00% 7 Missing ⚠️
.../io/opentelemetry/sdk/trace/SdkTracerProvider.java 0.00% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6899      +/-   ##
============================================
- Coverage     90.30%   90.25%   -0.06%     
- Complexity     6594     6595       +1     
============================================
  Files           729      729              
  Lines         19784    19799      +15     
  Branches       1945     1945              
============================================
+ Hits          17866    17869       +3     
- Misses         1325     1338      +13     
+ Partials        593      592       -1     

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


🚨 Try these New Features:

@jack-berg
Copy link
Member

Thanks for starting to think about this. I think we need to have an API that solves the dynamic config problem for generically, rather than focusing specifically on tracer config.

What do you think about something like this: jack-berg@f74db3e#diff-8a27a2383ac52aa80c19c1c9789bf6c91e2c336c702f9bb4943e5e8cce7d07e0R103-R129

Demonstrated in a test here: jack-berg@f74db3e#diff-cb1bd4f971c5bffe08539f05be0e05fe0750268975f448c45dcd78debd6775ecR257-R278

having a SdkTracerProvider.update(SdkTracerProvider target) method allows us to extend the method with additional update functionality as it makes sense. Could add similar method to SdkMeterProvider and SdkLoggerProvider, starting just with scope configuration, and slowly expanding to include other concepts like samplers, attribute limits, exporters, etc.

@jack-berg
Copy link
Member

Per our conversation in yesterday's SIG about having scope config part of declarative configuration, I've opened this PR: open-telemetry/opentelemetry-configuration#140

@jackshirazi
Copy link
Contributor Author

having a SdkTracerProvider.update(SdkTracerProvider target) method

Isn't this very specific to the tracer provider implementation? I get what you mean about it not just being for tracers, but that signature doesn't generalize to the TracerProvider interface. Also seems overkill to require the full SdkTracerProvider when you only want the (ScopeConfigurator) tracerConfigurator from that.

I can see the benefit of an update(ScopeConfigurator) which seems like it should be generic enough and could apply to the meter and logger providers, was there a reason to want the full provider?

@jack-berg
Copy link
Member

Isn't this very specific to the tracer provider implementation? I get what you mean about it not just being for tracers, but that signature doesn't generalize to the TracerProvider interface.

I meant that we need to think about: 1. All the parts of SdkTracerProvider that may need to be updated 2. How we do the same for SdkMeterProvider, SdkLoggerProvider.

Also seems overkill to require the full SdkTracerProvider when you only want the (ScopeConfigurator) tracerConfigurator from that.

Yeah its a matter of what type of API ergonomics we want.

Single update method for each provider

  • SdkTracerProvdider#update(SdkTracerProvider)
  • SdkMeterProvider#update(SdkMeterProvider)
  • SdkLoggerProvider#update(SdkLoggerProvider)

(Or alternatively, have the update methods accept a provider builder, i.e. SdkTracerProvider#update(SdkTracerProviderBuilder).)

  • Pros
    • Single method encapsulates all the configuration surface area.
    • The API stays constant as new providers evolve to include new components. The implementation of update changes as new components are added.
  • Cons
    • Need to construct a full provider just to update some small portion. For example, if you want to update only the ScopeConfigurator, your target SdkTracerProvider needs to include all processors, exporters, and samplers to make sure those don't get wiped out.
    • The set of updatable components is obscured as an implementation detail of the method. Callers need to go read the documentation to understand how it works.

Add setter methods for each updatable component

  • SdkTracerProvider

    • setScopeConfigurator(ScopeConfigurator)
    • setProcessors(Collection<SpanProcessors>)
    • setLimits(SpanLimits)
    • setSampler(Sampler)
  • SdkMeterProvider

    • setScopeConfigurator(ScopeConfigurator)
    • setReaders(Collection<MetricReaders>)
    • setExemplarFilter(ExemplarFilter)
    • setViews(Collection<Pair<InstrumentSelector, View>>)
  • SdkLoggerProvider

    • setScopeConfigurator(ScopeConfigurator)
    • setProcessors(Collection<LogRecordProcessors>)
    • setLimits(LogLimits)
  • Pros

    • Allows more surgical updates. No need for caller to understand the full configuration before to update a small piece.
    • Makes the set of components which are updatable explicit. If there's no set{Component}, it can't be updated. With the
  • Cons

    • Larger API surface area.
    • API need to be continuously added as providers evolve to include additional components
    • API design will be point of debate. Should we have even more methods to modify components rather than replacing wholesale? E.g. should we have methods to add a processor, or add a view, or limit to setting all processors, all views?

Listing out the methods and the pros / cons, I'm more in favor of the second approach. With the first approach of a single method, the caller needs to know what the complete configuration is when calling update, else risk accidentally removing configuration they didn't intended to impact. This is quite a burden to place on callers, and significantly worsens the ergonomics.

I was originally drawn to the first approach from the standpoint of maintainability - a single method is quite appealing from the standpoint of minimalism. But I think we can retain maintainability with the second approach if we have a policy to have a single setter set{Component} (i.e. setSampler) method for each component. We just avoid the temptation of adding all the syntactic sugar we have in the Sdk{Signal}ProviderBuilders.

@jackshirazi
Copy link
Contributor Author

I prefer the second approach too. It'll take me some time to find time (heh) and I'll convert this PR to that approach for the Tracers. I'd rather not do the meters and loggers just yet if that's okay?

@jack-berg
Copy link
Member

I'd rather not do the meters and loggers just yet if that's okay?

Yes let's establish the pattern in an initial, and then expand the scope. Should start with just setScopeConfigurator(ScopeConfigurator), rather than trying to do all the methods for SdkTracerProvider.

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