-
Notifications
You must be signed in to change notification settings - Fork 873
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
aws-sdk-1.1: Copy SQS plugin/NoMuzzle approach from 2.2 #8866
aws-sdk-1.1: Copy SQS plugin/NoMuzzle approach from 2.2 #8866
Conversation
Request<?> request, | ||
Response<?> response, | ||
Instrumenter<Request<?>, Response<?>> consumerInstrumenter) { | ||
if (response.getAwsResponse() instanceof ReceiveMessageResult) { |
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.
Previously we instead checked that the request is a ReceiveMessageRequest. This should not matter, but since we cast the response w/o expecting exceptions, I thought it safer to check the response instead.
(Build problems are unrelated to my changes: wrapper validation. (EDIT: Next build failing in different, also unrelated stages, e.g. smoke, ubuntu-latest, liberty: |
...ws-sdk-1.11/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/SqsImpl.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Mateusz Rzeszutek <[email protected]>
Yeah, this has happened in the daily build too. I'm thinking the GHA env might be a bit unstable right now. |
...1.11/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/PluginImplUtil.java
Outdated
Show resolved
Hide resolved
// (where it is always available) but a dependency failed to load (most likely because the | ||
// corresponding SDK dependency is not on the class path). | ||
logger.log( | ||
Level.FINE, e, () -> implFullClassName + " not present, probably incompatible version"); |
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.
nit: comment says that it probably fails because dependency is missing, log line says probably incompatible version
which feels inconsistent
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.
Adapted in c2cb01d
this one should be fixed in |
This basically is a port from the v2.2 instrumentation to v1.11 of #8603 (incl. follow-ups #8775 and #8805 and the AdviceBridge class move from #8830).
It does not contain any (intentional) functionality changes. At the moment it is a net code bloat (
13 files changed, 286 insertions(+), 255 deletions(-)
) but does add type safety & better muzzle coverage. Also, it should make porting further functionality changes for configured-propagator-based SQS/SNS features much more straightforward (and then result in a net reduction of LOC)