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

Instrumentation for Elasticsearch 8+ #8799

Merged
merged 14 commits into from
Jul 6, 2023
Merged

Instrumentation for Elasticsearch 8+ #8799

merged 14 commits into from
Jul 6, 2023

Conversation

AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented Jun 23, 2023 •
edited
Loading

New instrumentation module for Elasticsearch Clients 8+.

Fixes #8484

@AlexanderWert AlexanderWert requested a review from a team June 23, 2023 14:04
@AlexanderWert
Copy link
Member Author

AlexanderWert commented Jun 23, 2023 •
edited
Loading

Open TODOs

@AlexanderWert AlexanderWert mentioned this pull request Jun 24, 2023
Open
@AlexanderWert
test refactoring
74b8623 
Signed-off-by: Alexander Wert <[email protected]>
@AlexanderWert AlexanderWert force-pushed the es-8-instrumentation branch from 96d87e5 to 74b8623 Compare June 24, 2023 11:49
laurit
laurit reviewed Jun 26, 2023
View reviewed changes
...ion/elasticsearch/elasticsearch-rest-7.0/javaagent/src/test/java/ElasticsearchRest7Test.java Outdated Show resolved Hide resolved
instrumentation/elasticsearch/elasticsearch-api-client-8.0/javaagent/build.gradle.kts Outdated Show resolved Hide resolved
instrumentation/elasticsearch/elasticsearch-api-client-8.0/javaagent/build.gradle.kts Outdated Show resolved Hide resolved
...lemetry/javaagent/instrumentation/elasticsearch/apiclient/v8_0/ElasticsearchEndpointMap.java Outdated Show resolved Hide resolved
...lemetry/javaagent/instrumentation/elasticsearch/apiclient/v8_0/ElasticsearchEndpointMap.java Outdated Show resolved Hide resolved
...ticsearch/elasticsearch-api-client-8.0/javaagent/src/test/java/ElasticsearchClient8Test.java Outdated Show resolved Hide resolved
...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated Show resolved Hide resolved
...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated Show resolved Hide resolved
...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated Show resolved Hide resolved
...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated Show resolved Hide resolved
@AlexanderWert
Fixing comments from first round of review
2592d6a 
Signed-off-by: Alexander Wert <[email protected]>
@AlexanderWert
fixed style
befc456 
Signed-off-by: Alexander Wert <[email protected]>
@AlexanderWert AlexanderWert force-pushed the es-8-instrumentation branch from 1e4503e to d9c5117 Compare July 3, 2023 14:44
@laurit
Copy link
Contributor

laurit commented Jul 4, 2023

@AlexanderWert in case you didn't notice tests are failing when run against the latest version of the library, see https://scans.gradle.com/s/fwagc36gfpmv4/tests

@AlexanderWert
Copy link
Member Author

@AlexanderWert in case you didn't notice tests are failing when run against the latest version of the library, see https://scans.gradle.com/s/fwagc36gfpmv4/tests

fixing it

@AlexanderWert
Fixed tests
273a785 
Signed-off-by: Alexander Wert <[email protected]>
mateuszrzeszutek
mateuszrzeszutek reviewed Jul 4, 2023
View reviewed changes
...pentelemetry/javaagent/instrumentation/elasticsearch/apiclient/ElasticsearchEndpointMap.java Outdated Show resolved Hide resolved
...pentelemetry/javaagent/instrumentation/elasticsearch/apiclient/ElasticsearchEndpointMap.java Outdated Show resolved Hide resolved
...ntelemetry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchEndpointDefinition.java Outdated Show resolved Hide resolved
...ntelemetry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchEndpointDefinition.java Outdated Show resolved Hide resolved
...ntelemetry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchEndpointDefinition.java Show resolved Hide resolved
...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated Show resolved Hide resolved
...ticsearch/elasticsearch-api-client-7.16/javaagent/src/test/java/ElasticsearchClientTest.java Outdated Show resolved Hide resolved
...ticsearch/elasticsearch-api-client-7.16/javaagent/src/test/java/ElasticsearchClientTest.java Outdated Show resolved Hide resolved
...ticsearch/elasticsearch-api-client-7.16/javaagent/src/test/java/ElasticsearchClientTest.java Outdated Show resolved Hide resolved
...ion/elasticsearch/elasticsearch-rest-7.0/javaagent/src/test/java/ElasticsearchRest7Test.java Outdated Show resolved Hide resolved
@AlexanderWert
fixed review comments round 2
240f218 
Signed-off-by: Alexander Wert <[email protected]>
@AlexanderWert AlexanderWert force-pushed the es-8-instrumentation branch from 4d0f27b to 240f218 Compare July 4, 2023 13:23
@AlexanderWert
made attributeKeys cache bounded
f1fa9d2 
Signed-off-by: Alexander Wert <[email protected]>
@AlexanderWert
Copy link
Member Author

@mateuszrzeszutek, @laurit
Thanks for the reviews!
Do we need more approvals or can this be merged?

laurit
laurit approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@laurit laurit left a comment

Choose a reason for hiding this comment

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

You should check whether you need to update https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/supported-libraries.md#libraries--frameworks It lists Elasticsearch API and Elasticsearch REST Client Should it also have Elasticsearch Java API Client?

@laurit
Copy link
Contributor

laurit commented Jul 6, 2023

@mateuszrzeszutek, @laurit Thanks for the reviews! Do we need more approvals or can this be merged?

@AlexanderWert I think we can merge this once you have checked whether you need to update the supported libraries

@github-actions github-actions bot requested a review from theletterf July 6, 2023 09:40
@AlexanderWert
Copy link
Member Author

@mateuszrzeszutek, @laurit Thanks for the reviews! Do we need more approvals or can this be merged?

@AlexanderWert I think we can merge this once you have checked whether you need to update the supported libraries

Thanks @laurit , I've added the new instrumentation to the list of supported libraries

mateuszrzeszutek
mateuszrzeszutek approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

That's @AlexanderWert ! LGTM 👍

@mateuszrzeszutek mateuszrzeszutek enabled auto-merge (squash) July 6, 2023 09:47
@mateuszrzeszutek mateuszrzeszutek merged commit 6461f04 into open-telemetry:main Jul 6, 2023
@JonasKunz JonasKunz mentioned this pull request Jul 12, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

@laurit laurit laurit approved these changes

@theletterf theletterf Awaiting requested review from theletterf

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

Successfully merging this pull request may close these issues.

Adapt existing Elasticsearch instrumentation to comply with semconv proposal
3 participants
@AlexanderWert @laurit @mateuszrzeszutek