Skip to content
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-2.2: More reflection cleanup. #8775

Conversation

Oberon00
Copy link
Member

Follow-up to #8603, addressing some comments & finishing the half-done cleanup.

@Oberon00 Oberon00 requested a review from a team as a code owner June 20, 2023 16:27
@Oberon00

This comment was marked as resolved.

@Oberon00 Oberon00 marked this pull request as draft June 21, 2023 15:00
This is required to avoid enabled=false in some cases.
}
public void doTransform(TypeTransformer transformer) {
transformer.applyAdviceToMethod(
isConstructor(), SqsInstrumentationModule.class.getName() + "$RegisterAdvice");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isConstructor would apply to the constructor of software.amazon.awssdk.core.SdkClient which is an interface (https://github.com/aws/aws-sdk-java-v2/blob/c7c731f81d68403ce50b60dea61f413c269e57d0/core/sdk-core/src/main/java/software/amazon/awssdk/core/SdkClient.java#L27).

It does work though, and we would not depend on the Advice actually being invoked as long as the helper classes are injected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could also use none(), as you noted the isConstructor() match is impossible here, but as it is only needed to trigger class injection it does not need to match anyway. The none() to trigger injection trick is also used in OpenTelemetryInstrumentation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed that in a7683fd and some related cleanup.

@Oberon00
Copy link
Member Author

Build failures seem to be entirely unrelated, testLatestDeps (only) seems to consistently (or at least twice) fail in :instrumentation:mongo:mongo-4.0:javaagent:test. (Also, you might want to divide the tests up into yet more chunks, due to GH Action's truncation you cannot see the reason for failure without downloading the logs)

@Oberon00 Oberon00 marked this pull request as ready for review June 22, 2023 08:54
@laurit
Copy link
Contributor

laurit commented Jun 22, 2023

Build failures seem to be entirely unrelated, testLatestDeps (only) seems to consistently (or at least twice) fail in :instrumentation:mongo:mongo-4.0:javaagent:test. (Also, you might want to divide the tests up into yet more chunks, due to GH Action's truncation you cannot see the reason for failure without downloading the logs)

mongo latest dep test failure should be fixed with #8785 Latest dep tests test agains the latest version of the library, occasional breakage is expected when new library versions are released

Oberon00 and others added 2 commits June 22, 2023 16:29
…pentelemetry/instrumentation/awssdk/v2_2/SqsImpl.java

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants