-
Notifications
You must be signed in to change notification settings - Fork 674
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
SOLR-16938 Auto configure tracer without a <tracerConfig> tag in solr… #1922
Conversation
@janhoy if you have a few mins could you please take a look. The other part of this PR is passing the init params to the |
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.
Looks like a way to go.
Will you attempt a ref-guide edit as well?
@@ -49,17 +49,17 @@ public OtelTracerConfigurator() { | |||
|
|||
@Override | |||
public Tracer getTracer() { | |||
// TODO remove reliance on global | |||
return GlobalOpenTelemetry.getTracer("solr"); | |||
return TraceUtils.getGlobalTracer(); |
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.
Minor: While this is correct, it feels backward for a plugin method to ask a core class to return its instance.
solr/core/src/java/org/apache/solr/core/TracerConfigurator.java
Outdated
Show resolved
Hide resolved
String envValue = System.getenv().get(envName); | ||
String sysName = envName.toLowerCase(Locale.ROOT).replace("_", "."); | ||
String sysValue = System.getProperty(sysName); | ||
return sysValue != null ? sysValue : envValue; |
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.
This is duplicating OtelTracerConfigurator#getEnvOrSysprop
. Perhaps you can extract some functionality into a common static utility?
….xml
https://issues.apache.org/jira/browse/SOLR-16938
Description
Enable OTEL based on presence of system property. also use config params for otel init, but not if the respective system properties are already defined.
Solution
Please provide a short description of the approach taken to implement your solution.
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.