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

Should AutoConfigurationCustomizerProvider provide guarantees around the order in which customisations are processed? #4555

Closed
GrahamLea opened this issue Jun 23, 2022 · 6 comments · Fixed by #4609
Labels
Feature Request Suggest an idea for this project

Comments

@GrahamLea
Copy link

Is your feature request related to a problem? Please describe.
I've encountered this issue with Honeycomb's agent where their AutoConfigurationCustomizerProvider calls setSampler(), which overrides a Sampler which is installed as a decorator by an extension I've written.
Because AutoConfigurationCustomizerProviders are loaded and executed non-deterministically, and tracerProviderCustomizers are run after samplerCustomizers, there is no way to force my decorating Sampler to be installed after Honeycomb's override of the sampler.

Describe the solution you'd like
Either of the following changes in this project would make it possible to fix this particular problem:

  • Provide a means by which AutoConfigurationCustomizerProviders can be prioritised so that the order in which they're run is deterministic.
  • Apply samplerCustomizers after tracerProviderCustomizers, instead of before.

Describe alternatives you've considered
I've proposed a change to Honeycomb to make their own Sampler a decorator rather than an override.
That would solve my problem, but wouldn't solve the more generic problem that AutoConfigurationCustomizerProviders are ordered non-deterministically (I believe?) and can contain a mix of both decorating and overriding operations.

Additional context
More detail about the extension I'm creating is here, but probably isn't necessary to understand the issue: open-telemetry/opentelemetry-java-instrumentation#5008 (comment)

/cc @trask (who suggested raising this issue)

@jack-berg
Copy link
Member

We could add a default method to AutoConfigurationCustomizerProvider to indicate priority:

default int priority();

If this is a useful concept, we could extend it to other SPIs as needed, perhaps adding a super interface (i.e. Prioritizable).

I do think in this case honeycomb is perhaps making a stronger statement than they intend. By calling setSampler, they're essentially saying "ignore the autoconfigured sampler and set it this instead". Maybe that's the intent of their distro, but if they intend to offer more flexibility maybe they consider adding a ConfigurableSamplerProvider, and adding AutoConfigurationCustomizer#addPropertiesSupplier with properties that default to their sampler.

@mateuszrzeszutek
Copy link
Member

You could bring the javaagent's Ordered interface to this repo, and later on we'd remove our copy - I think it makes sense for the autoconfigure SPIs and javaagent extension SPIs to use the same pattern for ordering things.

@trask
Copy link
Member

trask commented Jun 30, 2022

You could bring the javaagent's Ordered interface to this repo, and later on we'd remove our copy - I think it makes sense for the autoconfigure SPIs and javaagent extension SPIs to use the same pattern for ordering things.

this would be nice I think

@anuraaga @jkwatson? (comment already got 👍 from @jack-berg)

@anuraaga
Copy link
Contributor

Yeah when someone needs ordering is when it's time to implement ordering. The javaagent interface looks good

@GrahamLea
Copy link
Author

I do think in this case honeycomb is perhaps making a stronger statement than they intend. By calling setSampler ...

I agree, and I think they did too - they've now merged a PR I produced to make their own Sampler a decorator rather than a replacement. Because the behaviours of their sampler and my sampler are commutative, the ordering doesn't matter for my specific case. (But I'm sure this wouldn't be true of all Samplers.)

From my POV as an end user, having to resort to getting a PR into a vendor's distro (and then waiting for a release), obviously doesn't feel like the right solution.

The suggestion about @Ordered above sounds good to me. If I'd had the ability to say "My config runs after Honeycomb's" that would've solved my issue without having to ask their distro to be changed.

An important point on implementation, though: The way AutoConfiguredOpenTelemetrySdkBuilder works would have to change quite a bit for ordering to have fixed my problem, I think.

Simply controlling the order in which customize() is called here:

for (AutoConfigurationCustomizerProvider customizer :
ServiceLoader.load(AutoConfigurationCustomizerProvider.class, serviceClassLoader)) {
customizer.customize(this);
}

would not have worked, because I install a "SamplerCustomizer", while Honeycomb was using a "TracerProviderCustomizer" to call setTracer(), which currently always executes after all SamplerCustomizers have been run:
TracerProviderConfiguration.configureTracerProvider(
tracerProviderBuilder,
config,
serviceClassLoader,
meterProvider,
spanExporterCustomizer,
samplerCustomizer);
tracerProviderBuilder = tracerProviderCustomizer.apply(tracerProviderBuilder, config);

So I think doing the ordering properly would mean getting the full suite of customisations in each AutoConfigurationCustomizerProvider (in order) and applying them all before moving to the next Provider. If there was ordering, but all SamplerCustomizers continued to be run before any TracerProviderCustomizers, my problem wouldn't have been solvable, because the TracerProviderCustomizers can trump anything.

Perhaps the power afforded to a TracerProviderCustomizer - of being able to set anything on a SdkTracerProviderBuilder (but being restricted from getting anything 🤔) - is a little dangerous?

@GrahamLea
Copy link
Author

GrahamLea commented Jul 27, 2022

Thanks @mateuszrzeszutek ! (And reviewers! 🙂)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants