added WithSpanInstrumentation #3406

videnkz · 2023-11-02T16:26:54Z

What does this PR do?

closes #2753

Added instrumentation for opentelemetry annotation WithSpan

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

TODO:
[x] - support of annotation WithSpan
[x] - support of annotation SpanAtribute

github-actions · 2023-11-02T16:27:16Z

👋 @videnkz Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

SylvainJuge

LGTM, I only have one minor question about custom annotation in tests.

...lugin/src/test/java/co/elastic/apm/agent/opentelemetry/tracing/ElasticOpenTelemetryTest.java

jackshirazi · 2023-11-20T21:56:22Z

CHANGELOG.asciidoc

@@ -38,6 +38,7 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
 * Capture bucket and object key to Lambda transaction as OTel attributes - `aws.s3.bueckt`, `aws.s3.key` - {pull}3364[#3364]
 * Added `context_propagation_only` configuration option - {pull}3358[#3358]
 * Added attribute[*] for JMX pattern metrics (all metrics can now be generated with `object_name[*:type=*,name=*] attribute[*]`) - {pull}3376[#3376]
+* Added support for OpenTelemetry annotations - `WithSpan` and `CaptureAttribute` - {pull}3406[#3406]


What is CaptureAttribute?

Hi @jackshirazi ,
My apologize, I fixed name of annotation: CaptureAttribute -> SpanAttribute.

jackshirazi · 2023-11-20T21:58:54Z

We should alter the CoreConfiguration.enablePublicApiAnnotationInheritance description line from

        .description("A boolean specifying if the agent should search the class hierarchy for public api annotations (`@CaptureTransaction`, `@CaptureSpan`, `@Traced`).\n " +

to

        .description("A boolean specifying if the agent should search the class hierarchy for public api annotations (`@CaptureTransaction`, `@CaptureSpan`, `@Traced` and from 1.44.0 `@WithSpan`).\n " +

…guration

videnkz · 2023-11-21T04:59:43Z

We should alter the CoreConfiguration.enablePublicApiAnnotationInheritance description line from

        .description("A boolean specifying if the agent should search the class hierarchy for public api annotations (`@CaptureTransaction`, `@CaptureSpan`, `@Traced`).\n " +

to

        .description("A boolean specifying if the agent should search the class hierarchy for public api annotations (`@CaptureTransaction`, `@CaptureSpan`, `@Traced` and from 1.44.0 `@WithSpan`).\n " +

updated

jackshirazi · 2023-11-23T09:20:59Z

@mergify rebase

mergify · 2023-11-23T09:21:10Z

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/9)
Rebasing (2/9)
Rebasing (3/9)
Rebasing (4/9)
Rebasing (5/9)
Auto-merging CHANGELOG.asciidoc
CONFLICT (content): Merge conflict in CHANGELOG.asciidoc
error: could not apply e86c762a0... added entry to changelog
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply e86c762a0... added entry to changelog

jackshirazi · 2023-11-23T13:25:07Z

@videnkz needs merging to main and ConfigurationExporterTest run to regenerate docs

…ations

videnkz · 2023-11-23T14:15:52Z

@videnkz needs merging to main and ConfigurationExporterTest run to regenerate docs

Done

jackshirazi · 2023-11-23T16:29:44Z

The test is failing, it doesn't fail locally but it looks like a real failure on CI, class visibility. I don't think I'm going to have time to look at fixing it this week, @videnkz if you have time please do

videnkz · 2023-11-24T09:10:22Z

The test is failing, it doesn't fail locally but it looks like a real failure on CI, class visibility. I don't think I'm going to have time to look at fixing it this week, @videnkz if you have time please do

I fixed it

jackshirazi · 2023-11-24T14:14:34Z

different failure now, again passes locally, failing in CI. Getting no spans for this in ElasticOpenTelemetryTest when executed from OpenTelemetryVersionIT

assertThat(reporter.getSpans()).hasSize(1);

videnkz · 2023-11-24T19:23:03Z

different failure now, again passes locally, failing in CI. Getting no spans for this in ElasticOpenTelemetryTest when executed from OpenTelemetryVersionIT
assertThat(reporter.getSpans()).hasSize(1);

@jackshirazi I did minor fixes in ElasticOpenTelemetryTest, I moved annotations related tests to separate test class. Can you check again plz?

jackshirazi · 2023-11-26T19:24:21Z

@elasticmachine run elasticsearch-ci/docs

JonasKunz · 2023-11-27T09:12:53Z

...lemetry-plugin/src/main/java/co/elastic/apm/agent/opentelemetry/WithSpanInstrumentation.java

+
+    @Override
+    public Collection<String> getInstrumentationGroupNames() {
+        return Collections.singletonList("opentelemetry-annotations");


The returned list should include super.getInstrumentationGroupNames(): Otherwise this instrumentation would stay enabled even if users added opentelemetry to the disabled instrumentations config option.

JonasKunz · 2023-11-27T10:36:16Z

...est/java/co/elastic/apm/agent/opentelemetry/tracing/ElasticOpenTelemetryAnnotationsTest.java

+        doStaff();
+    }
+
+    private void doStaff() {


Is this doStaff() really required? I think simply empty, annotated methods should be fine.

not required

JonasKunz · 2023-11-27T10:38:50Z

...ntelemetry-test/src/test/java/co/elastic/apm/agent/opentelemetry/OpenTelemetryVersionIT.java

@@ -140,4 +140,30 @@ void testUnsupportedMetricsSdkVersionsIgnored(String apiVersion, String sdkVersi
        runner.run();
    }

+    @ParameterizedTest
+    @ValueSource(strings = {


I think testing with just 1.20.0 should be fine here. The latest version will be run outside of this integration.
There were no changes between 1.20.0 and the latest one on the annotations (I think), so there is no need to spend time on testing them.

docs/api-opentelemetry.asciidoc

JonasKunz · 2023-11-27T10:41:50Z

CHANGELOG.asciidoc

@@ -44,6 +44,7 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
 * Capture bucket and object key to Lambda transaction as OTel attributes - `aws.s3.bueckt`, `aws.s3.key` - {pull}3364[#3364]
 * Added `context_propagation_only` configuration option - {pull}3358[#3358]
 * Added attribute[*] for JMX pattern metrics (all metrics can now be generated with `object_name[*:type=*,name=*] attribute[*]`) - {pull}3376[#3376]
+* Added support for OpenTelemetry annotations - `WithSpan` and `SpanAttribute` - {pull}3406[#3406]


Please move this up into a new section under the unreleased heading at the top, because 1.44.0 was released in the meantime:

=== Unreleased [float] ===== Features * Added support for OpenTelemetry annotations - `WithSpan` and `SpanAttribute` - {pull}3406[#3406]

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java

cla-checker-service · 2023-11-27T10:47:13Z

💚 CLA has been signed

Update apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java

Update docs/api-opentelemetry.asciidoc

Update apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java Update docs/api-opentelemetry.asciidoc

JonasKunz · 2023-11-27T12:08:36Z

@videnkz I think you need to rerun ConfigurationExporterTest locally and commit the resulting documentation changes: One of my suggestions altered the docs, so they need to be regenerated.

videnkz · 2023-11-27T12:17:44Z

@videnkz I think you need to rerun ConfigurationExporterTest locally and commit the resulting documentation changes: One of my suggestions altered the docs, so they need to be regenerated.

Done

JonasKunz · 2023-11-27T12:35:50Z

@elasticmachine run elasticsearch-ci/docs

JonasKunz · 2023-11-27T13:12:36Z

@videnkz Thanks for all the time & effort you invested to implement this! ❤️

royteeuwen · 2023-11-27T16:16:48Z

Very nice, thanks!

added WithSpanInstrumentation

92f1026

github-actions bot added agent-java community Issues and PRs created by the community triage labels Nov 2, 2023

videnkz added 5 commits November 2, 2023 22:41

minor polish with test

3c847f9

added handling of SpanAttribute annotation

64db577

minor test polish

07ffc57

added entry to changelog

e86c762

minor polish, updated doc

e9388a8

videnkz marked this pull request as ready for review November 3, 2023 11:56

videnkz added 2 commits November 3, 2023 19:13

override instrumentation group names

5f27998

merge conflicts resolved

2bc6c8d

SylvainJuge reviewed Nov 15, 2023

View reviewed changes

...lugin/src/test/java/co/elastic/apm/agent/opentelemetry/tracing/ElasticOpenTelemetryTest.java Outdated Show resolved Hide resolved

videnkz requested a review from SylvainJuge November 15, 2023 17:22

Merge branch 'main' into opentelemetry-annotations

a948efa

SylvainJuge previously approved these changes Nov 20, 2023

View reviewed changes

jackshirazi reviewed Nov 20, 2023

View reviewed changes

fixed annotation name in changelog entry

13b98cd

videnkz dismissed SylvainJuge’s stale review via 13b98cd November 21, 2023 04:56

updated description of enable_public_api_annotation_inheritance confi…

5ae9c57

…guration

videnkz requested a review from jackshirazi November 23, 2023 05:21

videnkz added 2 commits November 23, 2023 19:30

Merge remote-tracking branch 'upstream/main' into opentelemetry-annot…

1847e27

…ations

updated docs

313f687

changed access modifier to public

76f2724

JonasKunz requested changes Nov 27, 2023

View reviewed changes

videnkz added 3 commits November 27, 2023 17:15

moved opentelemetry annotations related tests to separate test class

75321ad

Update apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java

code review fixes

7b447a7

Update docs/api-opentelemetry.asciidoc

squash conflicts resolved

6623845

Update apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java Update docs/api-opentelemetry.asciidoc

videnkz force-pushed the opentelemetry-annotations branch from bd49c57 to 6623845 Compare November 27, 2023 11:31

deleted unecessary imports

4834d49

updated docs

d3ecec7

JonasKunz approved these changes Nov 27, 2023

View reviewed changes

jackshirazi approved these changes Nov 27, 2023

View reviewed changes

JonasKunz merged commit 59da6ee into elastic:main Nov 27, 2023
13 checks passed

JonasKunz mentioned this pull request Nov 27, 2023

let Corretto 21+ have non-redacted exceptions #3438

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added WithSpanInstrumentation #3406

added WithSpanInstrumentation #3406

videnkz commented Nov 2, 2023 •

edited

github-actions bot commented Nov 2, 2023

SylvainJuge left a comment

jackshirazi Nov 20, 2023

videnkz Nov 21, 2023

jackshirazi commented Nov 20, 2023

videnkz commented Nov 21, 2023

jackshirazi commented Nov 23, 2023

mergify bot commented Nov 23, 2023

jackshirazi commented Nov 23, 2023

videnkz commented Nov 23, 2023

jackshirazi commented Nov 23, 2023

videnkz commented Nov 24, 2023

jackshirazi commented Nov 24, 2023

videnkz commented Nov 24, 2023 •

edited

jackshirazi commented Nov 26, 2023

JonasKunz Nov 27, 2023

videnkz Nov 27, 2023

JonasKunz Nov 27, 2023

videnkz Nov 27, 2023

JonasKunz Nov 27, 2023

videnkz Nov 27, 2023

JonasKunz Nov 27, 2023

videnkz Nov 27, 2023

cla-checker-service bot commented Nov 27, 2023 •

edited

JonasKunz commented Nov 27, 2023

videnkz commented Nov 27, 2023

JonasKunz commented Nov 27, 2023

JonasKunz commented Nov 27, 2023

royteeuwen commented Nov 27, 2023

added WithSpanInstrumentation #3406

added WithSpanInstrumentation #3406

Conversation

videnkz commented Nov 2, 2023 • edited

What does this PR do?

Checklist

github-actions bot commented Nov 2, 2023

SylvainJuge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackshirazi commented Nov 20, 2023

videnkz commented Nov 21, 2023

jackshirazi commented Nov 23, 2023

mergify bot commented Nov 23, 2023

❌ Base branch update has failed

jackshirazi commented Nov 23, 2023

videnkz commented Nov 23, 2023

jackshirazi commented Nov 23, 2023

videnkz commented Nov 24, 2023

jackshirazi commented Nov 24, 2023

videnkz commented Nov 24, 2023 • edited

jackshirazi commented Nov 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-checker-service bot commented Nov 27, 2023 • edited

JonasKunz commented Nov 27, 2023

videnkz commented Nov 27, 2023

JonasKunz commented Nov 27, 2023

JonasKunz commented Nov 27, 2023

royteeuwen commented Nov 27, 2023

videnkz commented Nov 2, 2023 •

edited

videnkz commented Nov 24, 2023 •

edited

cla-checker-service bot commented Nov 27, 2023 •

edited