add method to retrieve instrumentation configuration by name #7927
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7927 +/- ##
============================================
- Coverage 90.10% 90.09% -0.02%
- Complexity 7370 7438 +68
============================================
Files 828 834 +6
Lines 22254 22541 +287
Branches 2192 2238 +46
============================================
+ Hits 20053 20309 +256
- Misses 1515 1532 +17
- Partials 686 700 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
api/incubator/src/main/java/io/opentelemetry/api/incubator/config/ConfigProvider.java
Outdated
Show resolved
Hide resolved
| * @param name the name of the instrumentation | ||
| * @return the {@link DeclarativeConfigProperties} for the given instrumentation name | ||
| */ | ||
| default DeclarativeConfigProperties getJavaInstrumentationConfig(String name) { |
There was a problem hiding this comment.
In the java SIG today we discussed adding an additional helper DeclarativeConfigProperties getGeneralInstrumentationConfig() which would operate the same but access the .instrumentation/development.general node.
Your call whether we do that in a separate PR or this one.
There was a problem hiding this comment.
Would be nice to also have a getDistributionConfig() to access the .distribution node (without null worries)
There was a problem hiding this comment.
That's a different story. Right now the scope of what ConfigProvider knows about is limited to the .instrumentation/development node. Would need to have something like open-telemetry/opentelemetry-specification#4800 to expand the scope.
Note its still possible for distributions to access .distribution without PR#4800 with a pattern like:
OpenTelemetryConfigurationModel model = parse(new File(System.getEnv("OTEL_EXPERIMENTAL_CONFIG_FILE")));
DistributionModel distribution = model.getDistribution();
ExtendedOpenTelemetrySdk sdk = create(model);
There was a problem hiding this comment.
that pattern is brittle, e.g. doesn't work with the spring starter
There was a problem hiding this comment.
@zeitlinger in spring starter can't we get the distribution node from https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/dcee1b769f353bee881ab7be8c7caa7715ea5143/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/OpenTelemetryAutoConfiguration.java#L158?
There was a problem hiding this comment.
Good point, that would work for spring applications.
AgentListenerpassesAutoConfiguredOpenTelemetrySdk- and that gives you access toConfigProvideronly - but Jacks suggestion would work- if the user also checks for the corresponding system property
...src/testIncubating/java/io/opentelemetry/sdk/autoconfigure/DeclarativeConfigurationTest.java
Show resolved
Hide resolved
zeitlinger
force-pushed
the
get-java-node
branch
from
d4d2658 to
051899e
Compare
|
@jack-berg added the second method for "general" and improved test coverage |
| * @return the {@link DeclarativeConfigProperties} for the given general instrumentation config | ||
| * name | ||
| */ | ||
| default DeclarativeConfigProperties getGeneralInstrumentationConfig(String name) { |
There was a problem hiding this comment.
This works because for now there are no top level properties under .instrumentation/development.general. But if there were, callers would be unable to access them. I think we should omit the String name parameter for now and just return the .instrumentation/development.general node.
There was a problem hiding this comment.
oh good point, can .instrumentation/development.java. have top-level properties?
I really don't mind getJavaInstrumentationConfig().get(name)
There was a problem hiding this comment.
This is only a shortcut method - so I think we're allowed to nudge towards what we think is best practice.
I think that is the currently implemented solution, but I can live with either.
There was a problem hiding this comment.
oh good point, can .instrumentation/development.java. have top-level properties?
No - its properties are limited to objects: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema-docs.md#experimentallanguagespecificinstrumentation-
In contrast, .general has explicit properties of http and peer which are objects, but nothing stating all properties are objects: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema-docs.md#experimentalgeneralinstrumentation-
There was a problem hiding this comment.
ok, I'll adapt "general" then
| * | ||
| * @return the {@link DeclarativeConfigProperties} for the general instrumentation configuration | ||
| */ | ||
| default DeclarativeConfigProperties getGeneralInstrumentationConfig() { |
There was a problem hiding this comment.
would we be comfortable with something shorter, e.g.
- getInstrumentationGeneral()
- getInstrumentationJava(name)
There was a problem hiding this comment.
What about "getGeneralInstrumentation"?
There was a problem hiding this comment.
What about "getGeneralInstrumentation"?
I like this for general, but symmetric naming yields getJavaInstrumentation(name), which makes it sound like its accessing the instrumentation rather than the config for the instrumentation.
Not that getInstrumentationJava(name) is especially obvious either, but at least the method name reflects the path to the YAML node being accessed.
There was a problem hiding this comment.
maybe
- getInstrumentation(name)
- getGeneralInstrumentation()
with the idea that the "Java" is redundant for the Java SDK
There was a problem hiding this comment.
Is getInstrumentation(name) too close to getInstrumentationConfig() given the difference in function?
There was a problem hiding this comment.
is there a way we could remove getInstrumentationConfig()?
what if we added something like .exists() to DeclarativeConfigurationProperties for the few people who may care about the difference between intermediate nodes being present or not
There was a problem hiding this comment.
I'm not particularly attached to it, but there's the bit about it being part of the spec. Of course the spec can be changed. But the path of least resistance for the short term is to find method names that jive with the current spec and which can be characterized as syntactic sugar. The spec doesn't prohibit syntactic sugar so it's the sweet spot where we can improve usability quickly.
There was a problem hiding this comment.
I'm not particularly attached to it, but there's the bit about it being part of the spec. Of course the spec can be changed. But the path of least resistance for the short term is to find method names that jive with the current spec and which can be characterized as syntactic sugar. The spec doesn't prohibit syntactic sugar so it's the sweet spot where we can improve usability quickly.
There was a problem hiding this comment.
I'm not particularly attached to it, but there's the bit about it being part of the spec. Of course the spec can be changed. But the path of least resistance for the short term is to find method names that jive with the current spec and which can be characterized as syntactic sugar. The spec doesn't prohibit syntactic sugar so it's the sweet spot where we can improve usability quickly.
There was a problem hiding this comment.
I went for
getInstrumentationConfiggetGeneralInstrumentationConfig
now based on the feedback. Let me know what you think.
3 participants
Follow up to #7923
See open-telemetry/opentelemetry-java-instrumentation#15641 (comment)