Skip to content

Instrumentation for Elasticsearch 8+#8799

Merged
mateuszrzeszutek merged 14 commits into
open-telemetry:mainfrom
AlexanderWert:es-8-instrumentation
Jul 6, 2023
Merged

Instrumentation for Elasticsearch 8+#8799
mateuszrzeszutek merged 14 commits into
open-telemetry:mainfrom
AlexanderWert:es-8-instrumentation

Conversation

@AlexanderWert

@AlexanderWert AlexanderWert commented Jun 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

New instrumentation module for Elasticsearch Clients 8+.

Fixes #8484

@AlexanderWert
Added Elasticsearch Client instrumentation for Elasticsearch 8+
74f4a31 
Signed-off-by: Alexander Wert <AlexanderWert@users.noreply.github.com>
@AlexanderWert
Fixed formatting
91fffda 
Signed-off-by: Alexander Wert <AlexanderWert@users.noreply.github.com>
@AlexanderWert AlexanderWert requested a review from a team June 23, 2023 14:04
@AlexanderWert

AlexanderWert commented Jun 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Open TODOs

@AlexanderWert
Moved Groovy tests to Java tests for ES Rest and API instrumentation
35bc09c 
Signed-off-by: Alexander Wert <AlexanderWert@users.noreply.github.com>
@AlexanderWert AlexanderWert mentioned this pull request Jun 24, 2023
Closed
@AlexanderWert
test refactoring
74b8623 
Signed-off-by: Alexander Wert <AlexanderWert@users.noreply.github.com>
@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
Comment thread ...ion/elasticsearch/elasticsearch-rest-7.0/javaagent/src/test/java/ElasticsearchRest7Test.java Outdated
Comment thread instrumentation/elasticsearch/elasticsearch-api-client-8.0/javaagent/build.gradle.kts
Comment thread instrumentation/elasticsearch/elasticsearch-api-client-8.0/javaagent/build.gradle.kts Outdated
Comment thread ...lemetry/javaagent/instrumentation/elasticsearch/apiclient/v8_0/ElasticsearchEndpointMap.java Outdated
Comment thread ...lemetry/javaagent/instrumentation/elasticsearch/apiclient/v8_0/ElasticsearchEndpointMap.java Outdated
Comment thread ...ticsearch/elasticsearch-api-client-8.0/javaagent/src/test/java/ElasticsearchClient8Test.java Outdated
Comment thread ...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated
Comment thread ...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated
Comment thread ...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated
Comment thread ...etry/javaagent/instrumentation/elasticsearch/rest/ElasticsearchClientAttributeExtractor.java Outdated
Alexander Wert added 2 commits June 26, 2023 16:45
Fixing comments from first round of review
2592d6a 
Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
fixed style
befc456 
Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
Refactored ES 7.16+ instrumentation
d9c5117 
Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@AlexanderWert AlexanderWert force-pushed the es-8-instrumentation branch from 1e4503e to d9c5117 Compare July 3, 2023 14:44
@laurit

laurit commented Jul 4, 2023

Copy link
Copy Markdown
Contributor

@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

AlexanderWert commented Jul 4, 2023

Copy link
Copy Markdown
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

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

AlexanderWert commented Jul 6, 2023

Copy link
Copy Markdown
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

@laurit laurit left a comment

Copy link
Copy Markdown
Contributor

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

laurit commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

@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

Added documentation of config option and supported libraries
4d521a1 
Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
@github-actions github-actions Bot requested a review from theletterf July 6, 2023 09:40
@AlexanderWert

AlexanderWert commented Jul 6, 2023

Copy link
Copy Markdown
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

@mateuszrzeszutek mateuszrzeszutek left a comment

Copy link
Copy Markdown
Member

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

@laurit laurit laurit approved these changes

@theletterf theletterf Awaiting requested review from theletterf

+1 more reviewer

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

Reviewers whose approvals may not affect merge requirements

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