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.

Sign up for GitHub

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

Jump to bottom

Separate out core instrumentation for AWS SDK to allow manual setup o… #421

Merged
merged 12 commits into from
May 27, 2020
Merged

Separate out core instrumentation for AWS SDK to allow manual setup o… #421

merged 12 commits into from
May 27, 2020

Conversation

anuraaga
Copy link
Contributor

…f instrumentation.

Related: #45

@trask Created this WIP PR exloring separating out the core instrumentation of AWS SDK so it can be used directly by users without the agent. Let me know if this is the direction you had in mind.

I haven't dug into the testing story yet, interestingly in this case, which may not apply for other instrumentation, the manual instrumentation misses out on the feature of being able to propagate context to Netty instrumentation (agent is doing quite an invasive AOP to enable it), so the agent and non-agent would need different test cases anyways.

Let me know if you have any thoughts!

/cc @shengxil

trask
trask reviewed May 21, 2020
View reviewed changes
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

Hi @anuraaga, thanks for working on this!

Good point about the aws-sdk instrumentation not being too typical compared to the other instrumentation.

What you have done is looking good to me.

Looking forward to seeing your thoughts about the testing story.

...java/io/opentelemetry/auto/instrumentation/awssdk/v2_2/AbstractAwsClientInstrumentation.java Show resolved Hide resolved
@anuraaga
Copy link
Contributor Author

I have added a simpler test runner for manual instrumentation tests (doesn't do any agenting). In the end, there is quite a bit of duplication in the test code, but it doesn't seem so bad. An idea is to accompany any e.g. aws-sdk-2.2-core module with aws-sdk-2.2-testing module with shared testing code, it's quite a bit of tooling though and depending on the instrumentation, may result in very little sharing.

I can't reproduce the build failure locally ><

@trask
Copy link
Member

trask commented May 21, 2020

Hey @anuraaga, it looks like the test failure is due to a file missing in your commit: InstrumentationTestRunner.java

@anuraaga
Copy link
Contributor Author

Ugh, sorry about that!

@trask
Copy link
Member

trask commented May 22, 2020

Hi @anuraaga, for the remaining build failures:

@anuraaga
Copy link
Contributor Author

Thanks for all these hints! For muzzle, unfortunately GitHub isn't recognizing that I did apply the change to the PR. Most of the muzzles started passing but now it gives Instrumentation unexpectedly passed Muzzle validation. Any idea what that might be about? Not sure what muzzle is doing yet...

@anuraaga
Copy link
Contributor Author

Was able to get the test to pass by updating the muzzle block, but I'm not sure what the implication of this is...

@anuraaga anuraaga marked this pull request as ready for review May 27, 2020 00:45
@anuraaga
Copy link
Contributor Author

Think this is finally ready for a look thanks for the patience

trask
trask reviewed May 27, 2020
View reviewed changes
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

I think this is a great start!

I propose we merge as-is (one minor suggestion below), and we can come back to remove the dependency on the auto-bootstrap module (e.g. the decorator hierarchy) once we have more clarity on the new "TypedSpan" hierarchy (from #195) which we are hoping to use to replace the decorator hierarchy. cc: @iNikem who is thinking about "TypedSpans" currently over in #229 and https://gitter.im/open-telemetry/opentelemetry-auto-instr-java?at=5ec7d8def0b8a2053aa83499.

instrumentation/aws-sdk/aws-sdk-2.2/aws-sdk-2.2.gradle Outdated Show resolved Hide resolved
@iNikem
Copy link
Contributor

iNikem commented May 27, 2020

I am not sure about code structure here. Having instrumentation and instrumentation-core as separate top level aggregates bothers me. I would very much prefer to have all code related to one library to be close together. Obvious way would be to have aws-sdk-2.2-auto and aws-sdk-2.2-manual, but I don't like it neither. What about aws-sdk-2.2 having code for manual integration and aws-sdk-2.2-auto for agent-based auto instrumentation?

@trask @tylerbenson @jkwatson what do you think?

@anuraaga
Copy link
Contributor Author

I definitely agree with that structure / naming and would prefer something like

instrumentation/
  aws-sdk-2.2/
    aws-sdk-2.2
    aws-sdk-2.2-auto
  ...

One reason I didn't do that in this PR is it looks like there would be a pretty big refactoring of the Gradle config where currently instrumentation configures all its subprojects with everything needed to build into the agent as auto instrumentation, so putting other projects in that folder currently wouldn't work well. Didn't want to tackle such a big change in this PR but let me know if it'd be better.

@iNikem
Copy link
Contributor

iNikem commented May 27, 2020

You are correct regarding Gradle scripts. Can you please add a TODO into README file of this new module? That it should be moved closer to "auto" module whenever build system supports it.

@anuraaga
Copy link
Contributor Author

Yup added it

trask
trask approved these changes May 27, 2020
View reviewed changes
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

👍 on our first adventure into side-by-side auto and manual instrumentation

@trask trask merged commit d4a14f6 into open-telemetry:master May 27, 2020
iNikem added a commit that referenced this pull request May 31, 2020
@iNikem @trask
@flands @malafeev @tylerbenson @mar-kolya @richardstartin @anuraaga
Rebase otel-java snapshot branch (#461)
72c9224 
* Define packages in exporter class loader (#409)

* Update docs about needing java 11 to build (#412)

* Update version to 0.3.0 (#413)

* Update the README (#414)

* Move build and configure to top as getting started section
* Add manual instrumentation section
* Document `@WithSpan` annotation
* Move developer specific information to CONTRIBUTING.md
* Cleanup formatting and use consistent spacing

* Update version to 0.4.0-SNAPSHOT (#415)

* Update CONTRIBUTING.md back to Java 11 (#419)

* Add Zipkin exporter support  (#411)

* #375 Add Zipkin exporter support

Signed-off-by: Sergei Malafeev <[email protected]>

* #375 use OkHttpSender for Zipkin exporter

Signed-off-by: Sergei Malafeev <[email protected]>

* #375 add Zipkin exporter to README

Signed-off-by: Sergei Malafeev <[email protected]>

Co-authored-by: Trask Stalnaker <[email protected]>

* Remove inactive maintainer (#420)

* Fix brolen anchor link (#422)

* Fix khttp instrumentation in case of absent or read-only headers map (#416)

* Add new approver (#429)

* Fix sqlNormalizerEnabled initialization (#432)

* Remove unused config (#424)

* Change names of servlet based server spans (#428)

* Add documentation describing non-obvious points of Servlet instrumentations

* Change names of servlet based server spans

* Fix java google format (#439)

* Remove deprecated usage from internal instrumentation (DataDog/dd-trace-java#1441)

* Update gradle to 6.4 (DataDog/dd-trace-java#1443)

* Migrate lettuce instrumentation away from deprecated finishSpanOnClose (DataDog/dd-trace-java#1445)

* Make Retrys consistent (DataDog/dd-trace-java#1442)

* Rename java packages for lettuce 4 and 5 to not have collisions (DataDog/dd-trace-java#1450)

* Adding an option to manually disable Kafka headers (DataDog/dd-trace-java#1448)

* Add version specific names to allow disabling only a specific version (DataDog/dd-trace-java#1456)

* Attempt to improve muzzle time by randomly ignoring versions until 10 remain (DataDog/dd-trace-java#1451)

* Save circle cache with name matching restore patterns (DataDog/dd-trace-java#1457)

* Wrap log statements using varargs to avoid object allocation (DataDog/dd-trace-java#1466)

* Grizzly-http and grizzly-client instrumentation (DataDog/dd-trace-java#1365)

* More refactoring for ScopeManager (DataDog/dd-trace-java#1467)

* Remove Java 9 and 10 from the build (DataDog/dd-trace-java#1473)

* Disable CI cache for muzzle (DataDog/dd-trace-java#1479)

* Separate out core instrumentation for AWS SDK to allow manual setup o… (#421)

* Separate out core instrumentation for AWS SDK to allow manual setup of instrumentation.

* Instrumentation core test

* Fix sporadic Elasticsearch test failures (#444)

* Fix sporadic grizzly test failure (#448)

* Fixes integration with latest version of Finatra (#450)

* #425 allow only one propagator per concern (#451)

* Updates to reflect new repo name (#454)

* Remove printlns that were accidentally committed (#459)

* Remove unnecessary version constant (#455)

Co-authored-by: Trask Stalnaker <[email protected]>
Co-authored-by: Steve Flanders <[email protected]>
Co-authored-by: Sergei Malafeev <[email protected]>
Co-authored-by: Tyler Benson <[email protected]>
Co-authored-by: Nikolay Martynov <[email protected]>
Co-authored-by: Brian Devins-Suresh <[email protected]>
Co-authored-by: Richard Startin <[email protected]>
Co-authored-by: Anuraag Agrawal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trask trask trask approved these changes

@iNikem iNikem Awaiting requested review from iNikem

@jkwatson jkwatson Awaiting requested review from jkwatson

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anuraaga @trask @iNikem