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 instrumentation for S3 and DynamoDB #2606

Merged
merged 8 commits into from
May 6, 2022

Conversation

AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented May 2, 2022

What does this PR do?

Instrumentation for AWS SDK covering S3 and DynamoDB

Closes #1636
Closes #1637

Checklist

  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.

@apmmachine
Copy link
Contributor

apmmachine commented May 2, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-05-06T06:01:22.867+0000

  • Duration: 54 min 36 sec

Test stats 🧪

Test Results
Failed 0
Passed 2869
Skipped 22
Total 2891

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@Nullable
private static DynamoDbHelper INSTANCE;

public static DynamoDbHelper getInstance() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Although unlikely, there could be concurrent calls that would create multiple instances, which could produce different threads using different req-table maps. This would quickly be resolved for subsequent requests, so it is only a matter of some odd initial reqs being handled incorrectly in the worst case. So can leave it like this, especially if the lazy initialization is deliberate. An alternative is to initialize at class initialization time

Copy link
Member Author

@AlexanderWert AlexanderWert May 6, 2022

Choose a reason for hiding this comment

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

I think having the lazy initialization is good to not allocate unnecessary memory in case certain parts of the SDK are not used at all (e.g. S3 but not DynamoDB).
Regarding race, I agree that it's possible but very unlikely and also not a big problem when it occurs. And actually we do the same in other plugins already. I think introducing a synchronized block is not worth the benefit compared to the performance impact. So I would leave it as is.

Copy link
Contributor

@jackshirazi jackshirazi left a comment

Choose a reason for hiding this comment

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

nice!

@AlexanderWert AlexanderWert enabled auto-merge (squash) May 6, 2022 06:02
@AlexanderWert AlexanderWert merged commit a1f1e18 into elastic:main May 6, 2022
videnkz pushed a commit to videnkz/apm-agent-java that referenced this pull request May 10, 2022
* AWS SDK instrumentation for S3 and DynamoDB

* fixed Java 7 compatibility problems

* added license headers

* refactored test to use Java 8 Collection utils

* Update configuration.asciidoc

* refactored abstract test class for aws sdk

* Update CHANGELOG.asciidoc to add S3 and DynamoDB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 408] Instrumentation for AWS S3 [META 408] Instrumentation for DynamoDB
3 participants