-
Notifications
You must be signed in to change notification settings - Fork 900
add config model customizer #7118
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 config model customizer #7118
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7118 +/- ##
============================================
+ Coverage 89.93% 89.97% +0.03%
- Complexity 6676 6689 +13
============================================
Files 750 751 +1
Lines 20168 20187 +19
Branches 1978 1982 +4
============================================
+ Hits 18138 18163 +25
+ Misses 1435 1432 -3
+ Partials 595 592 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...ry/sdk/extension/incubator/fileconfig/OpenTelemetryConfigurationModelCustomizerProvider.java
Outdated
Show resolved
Hide resolved
…tension/incubator/fileconfig/OpenTelemetryConfigurationModelCustomizerProvider.java Co-authored-by: jack-berg <[email protected]>
0700275
to
5fd3e48
Compare
@jack-berg the only failing check is transient - PR can be reviewed 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small comments, but a good start and something we can iterate on.
Thanks!
.../io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfigurationCustomizer.java
Show resolved
Hide resolved
.../io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfigurationCreateTest.java
Outdated
Show resolved
Hide resolved
Function<? super I, ? extends O1> first, Function<? super O1, ? extends O2> second) { | ||
return (I configured) -> { | ||
O1 firstResult = first.apply(configured); | ||
return second.apply(firstResult); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we might consider adding opt-in logging to print the out change in the model after each stage of customization, but I'm happy to come back to that type of thing later. This is a good start!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's leave that as a next step then
* configuration. | ||
* | ||
* @param customizer the customizer to add | ||
*/ | ||
void addModelCustomizer( | ||
Function<OpenTelemetryConfigurationModel, OpenTelemetryConfigurationModel> customizer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realizing that this is going to be a problematic contract from a packaging standpoint..
- Currently, the SPI lives in
opentelemetry-sdk-extension-incubator
- In the future, we'll want to move it to
opentelemetry-sdk-extension-autoconfigure-spi
with the rest of the SPIs - That leaves a thorny question for where
OpenTelemetryConfigurationModel
and the rest of the data model lives. First guess would beopentelemetry-sdk-extension-autoconfigure
, but that creates a dependency cycle. They could live inopentelemetry-sdk-extension-autoconfigure-spi
, oropentelemetry-sdk
, oropentelemetry-sdk-common
, or some newopentelemetry-sdk-config
package.
Idk, let's cross that bridge when we get there.
Relates open-telemetry/opentelemetry-specification#4416
Adds a hook for distributions to customize the parsed configuration.