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

Added ES client 8.x instrumentation #3157

Merged
merged 12 commits into from
Jul 26, 2023
Merged

Added ES client 8.x instrumentation #3157

merged 12 commits into from
Jul 26, 2023

Conversation

AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented May 31, 2023 •
edited by JonasKunz
Loading

What does this PR do?

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation

@apmmachine
Copy link
Contributor

apmmachine commented May 31, 2023 •
edited
Loading

💚 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 previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-06-12T15:39:17.476+0000

  • Duration: 14 min 59 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the 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!)

@AlexanderWert AlexanderWert requested a review from JonasKunz June 1, 2023 14:03
@AlexanderWert
added plugin to agent-builds pom
e83ba9b 
Signed-off-by: Alexander Wert <[email protected]>
JonasKunz
JonasKunz reviewed Jun 7, 2023
View reviewed changes
.../src/main/java/co/elastic/apm/agent/esapiclient/v8_x/RestClientTransportInstrumentation.java Outdated Show resolved Hide resolved
apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-8_x/pom.xml Outdated Show resolved Hide resolved
...-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/EndpointResolutionHelper.java Outdated
private EndpointResolutionHelper() {
// This map is generated from the Java client code.
// It's a one-time generation that shouldn't require maintenance effort,
// as in the future the ES client will come with a native instrumentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be a way for us to detect that the used ES client already comes with OTel instrumentation? So that we can disable our own ES-client instrumentation? E.g. a newly added class for whose presence we can test would be perfect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added issue as follow up task, because the native ES client is not merged yet:
#3235

...-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/EndpointResolutionHelper.java Outdated Show resolved Hide resolved
...-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/EndpointResolutionHelper.java Outdated Show resolved Hide resolved
...-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/EndpointResolutionHelper.java Outdated Show resolved Hide resolved
...client-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/RequestEndpointMap.java Outdated Show resolved Hide resolved
...mon/src/test/java/co/elastic/apm/agent/esrestclient/AbstractEsClientInstrumentationTest.java Outdated Show resolved Hide resolved
...mon/src/test/java/co/elastic/apm/agent/esrestclient/AbstractEsClientInstrumentationTest.java Outdated Show resolved Hide resolved
@JonasKunz JonasKunz self-assigned this Jul 7, 2023
JonasKunz added 7 commits July 7, 2023 11:09
@JonasKunz
Review fixes
1a68fc6
@JonasKunz
Copied otel classes
046e619
@JonasKunz
Updated to match latest OTel spec
103fcb0
@JonasKunz
Fixed and refactored test validations
befe127
@JonasKunz
Added missing test
b50352f
@JonasKunz
Merge remote-tracking branch 'elastic/main' into es-8.x
ccbd6a0 
# Conflicts:
#	apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-8_x/pom.xml
#	apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/ElasticsearchRestClientInstrumentationHelper.java
@JonasKunz
Added missing comments
50da37c
@JonasKunz
Copy link
Contributor

Newest updates:

  • ElasticsearchEndpointDefinition and ElasticsearchEndpointMap have been taken from the corresponding OTel plugin. Slight adjustments were needed due to Java 7 support
  • The plugin has been adapted to correctly set the db.operation and db.elasticsearch.path_parts.* attributes
  • If the endpoint-name is available (ES 7.16+), the span name will be Elasticsearch: <endpointName>
  • If the endpoint-name is unavailable the span name will be Elasticsearch: <method> <http-path> (like it was before)

I had quiet a hard time grasping all the indirections with flags in the tests. That's why I decided to refactor them to use a validation builder (validateSpan()..check()), which should make the tests much easier to understand without having to jump to method definitions.

@JonasKunz JonasKunz requested a review from jackshirazi July 12, 2023 13:02
jackshirazi
jackshirazi approved these changes Jul 26, 2023
View reviewed changes
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, I like the new test validation formats, it's clearer

@JonasKunz JonasKunz enabled auto-merge (squash) July 26, 2023 09:45
@JonasKunz JonasKunz merged commit 7d59455 into elastic:main Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonasKunz JonasKunz JonasKunz left review comments

@jackshirazi jackshirazi jackshirazi approved these changes

Assignees

@JonasKunz JonasKunz

Labels
agent-java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexanderWert @apmmachine @JonasKunz @jackshirazi