Skip to content

Capture the SNS topic ARN under the 'messaging.destination.name' span attribute.#10096

Merged
trask merged 10 commits into
open-telemetry:mainfrom
tduncan:feat-sns-topic-arn
Jan 3, 2024
Merged

Capture the SNS topic ARN under the 'messaging.destination.name' span attribute.#10096
trask merged 10 commits into
open-telemetry:mainfrom
tduncan:feat-sns-topic-arn

Conversation

@tduncan

@tduncan tduncan commented Dec 18, 2023

Copy link
Copy Markdown
Contributor

Addresses #10030

@tduncan
tduncan requested a review from a team December 18, 2023 19:38
@linux-foundation-easycla

linux-foundation-easycla Bot commented Dec 18, 2023 •
edited
Loading

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

@laurit

laurit commented Dec 19, 2023

Copy link
Copy Markdown
Contributor

Should the same change also be made for aws 1 instrumentation? Ideally I think we should change the SNS spans to follow messaging semantic conventions like we did for SQS, though this is probably too much effort and out of scope for this issue.

@tduncan

tduncan commented Dec 19, 2023

Copy link
Copy Markdown
Contributor Author

@laurit, I'd be happy to update the AWS SDK v1 instrumentation as well though I don't see explicit support for SNS as I do in the v2 instrumentation.

@laurit

laurit commented Dec 20, 2023

Copy link
Copy Markdown
Contributor

@tduncan I think in aws1 sdk you'd need to add an accessor in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/aws-sdk/aws-sdk-1.11/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/RequestAccess.java that lets you reflectively retrieve the value and create an attribute extractor similar to https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/aws-sdk/aws-sdk-1.11/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AwsSdkExperimentalAttributesExtractor.java Can't use AwsSdkExperimentalAttributesExtractor because experimental (non spec) attributes are collected only when enabled with a flag. And add that extractor to

opentelemetry-java-instrumentation/instrumentation/aws-sdk/aws-sdk-1.11/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AwsSdkInstrumenterFactory.java

Line 44 in 2199cc5

defaultAttributesExtractors = Arrays.asList(httpAttributesExtractor, rpcAttributesExtractor);
and

opentelemetry-java-instrumentation/instrumentation/aws-sdk/aws-sdk-1.11/library/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AwsSdkInstrumenterFactory.java

Line 48 in 2199cc5

httpAttributesExtractor, rpcAttributesExtractor, experimentalAttributesExtractor);
Hopefully that would be enough (besides aws1 tests also run camel-2.20 tests, these also rely on the aws1 instrumentation)

@tduncan

tduncan commented Dec 21, 2023

Copy link
Copy Markdown
Contributor Author

@laurit, thanks for the tips! I'll explore the v1 SDK instrumentation in more detail.

laurit
laurit reviewed Dec 22, 2023
View reviewed changes
Comment thread ...rary/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/SnsAttributesExtractor.java Outdated
@Override
public void onStart(AttributesBuilder attributes, Context parentContext, Request<?> request) {
String destination = findMessageDestination(request.getOriginalRequest());
if (destination != null) {

@laurit laurit Dec 22, 2023

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.

Usually we use

opentelemetry-java-instrumentation/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/AttributesExtractorUtil.java

Lines 22 to 27 in 1ee51e4

public static <T> void internalSet(
AttributesBuilder attributes, AttributeKey<T> key, @Nullable T value) {
if (value != null) {
attributes.put(key, value);
}
}
which does the null check

@trask
trask merged commit 1c1ca91 into open-telemetry:main Jan 3, 2024
@jaydeluca jaydeluca mentioned this pull request Jan 4, 2024
Merged
@laurit laurit mentioned this pull request Jan 5, 2024
Closed
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
@zeitlinger zeitlinger zeitlinger approved these changes
+1 more reviewer
@kenfinnigan kenfinnigan kenfinnigan 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.

5 participants

@tduncan @laurit @kenfinnigan @zeitlinger @trask