feat(config): Add exporter customizers for declarative config (#6576)#8081
feat(config): Add exporter customizers for declarative config (#6576)#8081MikeGoldsmith wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
- Add SpanExporter/MetricExporter/LogRecordExporter customizers to DeclarativeConfigurationCustomizer - Customizers compose in SPI registration order - Apply in factories after component creation, before return - Null returns throw DeclarativeConfigException - Tests: factory unit tests, builder composition tests, integration tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8081 +/- ##
============================================
+ Coverage 90.20% 90.22% +0.02%
- Complexity 7595 7618 +23
============================================
Files 841 841
Lines 22919 22973 +54
Branches 2291 2294 +3
============================================
+ Hits 20674 20728 +54
Misses 1529 1529
Partials 716 716 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nto mike/exporter-customizers-clean
jack-berg
left a comment
There was a problem hiding this comment.
I'm supportive of these customizers because even though we want to encourage modeling all properties that matter in the opentelemetry-configuration schema, it will never be possible to cover all cases (e.g. think the executor service used to eval async requests), and we'll want an escape hatch to leverage when a a property hasn't yet been modeled in declarative config but will be.
The exporters are a good place to start because there are known gaps in the what can be expressed in declarative config (i.e. authenticators).
Let's hold off modeling additional customizers (e.g. span processors, propagators, etc) until there are concrete requests by end users or distributions which require them.
| } | ||
|
|
||
| // Pass exporter customizers to context | ||
| context.setSpanExporterCustomizer(builder.getSpanExporterCustomizer()); |
There was a problem hiding this comment.
Maybe we can just pass the builder to DeclarativeConfigContext to avoid the noise of extra getters / setter for each customizer?
| * | ||
| * @param customizer function receiving (exporterName, exporter) and returning customized exporter | ||
| */ | ||
| void addSpanExporterCustomizer(BiFunction<String, SpanExporter, SpanExporter> customizer); |
There was a problem hiding this comment.
This contract is different than the env var / system prop equivalent:
AutoConfigurationCustomizer addSpanExporterCustomizer(BiFunction<SpanExporter, ConfigProperties, SpanExporter>)
Comments:
- There are mechanisms for a customizer to identify the exporter and decide whether to participate: the exporterName and the class type of the exporter. We don't technically need both. What's the advantage (if any) of the exporter name in addition to the type?
- The customizer doesn't have access to the
DeclarativeConfigPropertiesfor the exporter. We need to add this. - One annoying part about this and the existing AutoConfigurationCustomizer pattern is that the customizer implementation has to filter all span exporter and only operate on the instances they care about. I wonder if we could improve on this by decomposing it into a predicate and a customizer:
addSpanExporterCustomizer(Predicate<SpanExporter>, BiFunction<SpanExporter, DeclarativeConfigProperties, SpanExporter>, or even something like:<T extends SpanExporter> addSpanExporterCustomizer(Class<T>, BiFunction<T, DeclarativeConfigProperties, T>)to remove the need to do this filtering and type casting entirely.
Summary
Adds exporter customizers to
DeclarativeConfigurationCustomizerfor programmatic customization ofSpanExporter,MetricExporter, andLogRecordExporterinstances created from declarative configuration.Fixes #6576
Changes
addSpanExporterCustomizer(),addMetricExporterCustomizer(),addLogRecordExporterCustomizer()toDeclarativeConfigurationCustomizermergeBiFunctionCustomizer()inDeclarativeConfigurationBuilder