-
Notifications
You must be signed in to change notification settings - Fork 791
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
Muzzle should add SPI classes defined in helperResourceNames as references #1918
Conversation
"io.opentelemetry.instrumentation.awssdk.v2_2.TracingExecutionInterceptor", | ||
"io.opentelemetry.extension.trace.propagation.AwsXRayPropagator" | ||
}; | ||
return new String[] {"io.opentelemetry.extension.trace.propagation.AwsXRayPropagator"}; |
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.
Thanks a lot for the change!!! Just checking, will muzzle task fail if this class isn't included now?
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.
Yes, it will:
FAILED MUZZLE VALIDATION: io.opentelemetry.javaagent.instrumentation.awssdk.v2_2.AwsSdkInstrumentationModule mismatches:
-- io.opentelemetry.instrumentation.awssdk.v2_2.AwsSdkHttpClientTracer:39 Missing class io.opentelemetry.extension.trace.propagation.AwsXRayPropagator
} | ||
|
||
List<String> spiImplementations = new ArrayList<>(); | ||
try (InputStream stream = getResourceStream(resource)) { |
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.
Is it possible to use Files.lines
or Files.readAllLines
?
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.
I wanted to use one of those but they all accept Path
- and we pretty much have to use InputStream
here because of connection.setUseCaches(false)
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 great! will really encourage us to instrument via official SPI hooks more 👍
Resolves #1887