[Instrumentation.AWS] Update the instrumentation logic to use AWS TracerProvider. by muhammad-othman · Pull Request #1974 · open-telemetry/opentelemetry-dotnet-contrib

muhammad-othman · 2024-07-24T21:57:31Z

Changes

This PR focuses on leveraging the newly introduced tracing APIs within the AWS SDK. The main changes include:

Integration of New Tracing APIs:
Implemented the new AWS SDK tracing APIs and updated the AddAWSInstrumentation method to register this implementation using AWSTracerProvider.
Simplification of Tracing Logic:
Removed AWSTracingPipelineHandler and migrated its logic to either the .NET SDK or AWSPropagatorPipelineHandler.
SDK Version Update:
Updated the SDK version to 3.7.400 to incorporate the new tracing APIs.

Merge requirement checklist

Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes

linux-foundation-easycla · 2024-07-24T21:57:38Z

The committers listed above are authorized under a signed CLA.

✅ login: muhammad-othman / name: Muhammad Othman (eba2928, 2cf1ec6, f99bf7d, e88b210, 17c295a)

Kielek · 2024-07-25T04:41:08Z

@muhammad-othman, please sign EasyCLA. Without this we are not allowed to proceed with the PR.

normj · 2024-07-31T17:14:46Z

With the previous implementation we had the following AWS attributes added on the span

And with the new implementation we are only getting queue url and request id.

Can we get operation, region and service added on the spans in the new attribute?

muhammad-othman · 2024-08-01T15:22:41Z

With the previous implementation we had the following AWS attributes added on the span

And with the new implementation we are only getting queue url and request id.

Can we get operation, region and service added on the spans in the new attribute?

@normj
Yep, the following attributes was removed since they aren't part of OTel specs

I believe they were added them preemptively before they were added to the specs, however now we have rpc.method and rpc.service to replace aws.service and aws.operation attributes
https://opentelemetry.io/docs/specs/semconv/cloud-providers/aws-sdk/#common-attributes

The only attribute that I removed is aws.region which doesn't have any replacement in the specs yet, not sure if we should keep it knowing that it may be added in a different name to specs, and if we will keep it, shouldn't it get added to the other SDKs too?

normj · 2024-08-01T19:02:22Z

I believe they were added them preemptively before they were added to the specs, however now we have rpc.method and rpc.service to replace aws.service and aws.operation attributes
https://opentelemetry.io/docs/specs/semconv/cloud-providers/aws-sdk/#common-attributes

I missed that the service and operation were moved to OTel specific attributes. Region probably doesn't make sense at the OTel spec level since that is more of an AWS attribute. We should discuss with the rest of the SDK if we should add an aws.region attribute to our standardization. That isn't really tied to this PR so we don't need to hold this PR back for that.

normj · 2024-08-01T19:04:03Z

test/OpenTelemetry.Instrumentation.AWS.Tests/TestAWSClientInstrumentation.cs

    private void ValidateDynamoActivityTags(Activity ddb_activity)
    {
        Assert.Equal("DynamoDB.Scan", ddb_activity.DisplayName);
-        Assert.Equal("DynamoDB", Utils.GetTagValue(ddb_activity, "aws.service"));


Instead of removing these checks for aws.service and aws.operation shouldn't they updated to check rpc.service and rpc.method?

Yep, they do exist here

normj · 2024-08-01T19:04:19Z

test/OpenTelemetry.Instrumentation.AWS.Tests/TestAWSClientInstrumentation.cs

    private void ValidateSqsActivityTags(Activity sqs_activity)
    {
        Assert.Equal("SQS.SendMessage", sqs_activity.DisplayName);
-        Assert.Equal("SQS", Utils.GetTagValue(sqs_activity, "aws.service"));


Instead of removing these checks for aws.service and aws.operation shouldn't they updated to check rpc.service and rpc.method?

Yep, they do exist here

cijothomas · 2024-08-05T17:30:46Z

@srprash Could you review?

codecov · 2024-08-05T17:36:52Z

Codecov Report

Attention: Patch coverage is 69.14894% with 29 lines in your changes missing coverage. Please review.

Project coverage is 75.77%. Comparing base (71655ce) to head (17c295a).
Report is 389 commits behind head on main.

Files	Patch %	Lines
...ntation.AWS/Implementation/Tracing/AWSTraceSpan.cs	56.75%	16 Missing ⚠️
...umentation.AWS/Implementation/Tracing/AWSTracer.cs	60.00%	10 Missing ⚠️
...AWS/Implementation/AWSPropagatorPipelineHandler.cs	88.88%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1974      +/-   ##
==========================================
+ Coverage   73.91%   75.77%   +1.85%     
==========================================
  Files         267       20     -247     
  Lines        9615      227    -9388     
==========================================
- Hits         7107      172    -6935     
+ Misses       2508       55    -2453

Flag	Coverage Δ
unittests-Instrumentation.AWS	`75.77% <69.14%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...rumentation.AWS/AWSClientInstrumentationOptions.cs	`100.00% <100.00%> (ø)`
...AWS/Implementation/AWSTracingPipelineCustomizer.cs	`88.88% <100.00%> (+1.38%)`	⬆️
...ntation.AWS/Implementation/Metrics/AWSHistogram.cs	`71.42% <ø> (ø)`
...rumentation.AWS/Implementation/Metrics/AWSMeter.cs	`100.00% <ø> (ø)`
...ion.AWS/Implementation/Metrics/AWSMeterProvider.cs	`100.00% <ø> (ø)`
....AWS/Implementation/Metrics/AWSMonotonicCounter.cs	`71.42% <ø> (ø)`
...ion.AWS/Implementation/Metrics/AWSUpDownCounter.cs	`85.71% <ø> (ø)`
...on.AWS/Implementation/Tracing/AWSTracerProvider.cs	`100.00% <100.00%> (ø)`
...rumentation.AWS/TracerProviderBuilderExtensions.cs	`100.00% <100.00%> (ø)`
...AWS/Implementation/AWSPropagatorPipelineHandler.cs	`93.18% <88.88%> (ø)`
... and 2 more

... and 265 files with indirect coverage changes

martincostello · 2024-08-09T11:55:23Z

src/OpenTelemetry.Instrumentation.AWS/Implementation/Tracing/AWSTraceSpan.cs

+
+namespace OpenTelemetry.Instrumentation.AWS.Implementation.Tracing;
+
+internal class AWSTraceSpan : TraceSpan


Make AWSTraceSpan, AWSTracer and AWSTracerProvider sealed to aid with JIT optimisations?

Updated them and updated the metrics classes too.

…cerProvider.

…ions

birojnayak · 2024-08-13T01:28:58Z

aws.region is missing as pointed out earlier, while others have replacement. See if you can have, because existing customers/users might be relying on it to build custom dashboard or extensions.

muhammad-othman · 2024-08-13T19:26:56Z

@birojnayak Unfortunately aws.region attribute doesn't currently exist in the other AWS SDKs and there is no replacement for it in the current specifications. My concern is that if we add this attribute now, we may later reach an agreement with other SDKs on a different name, similar to how aws.service and aws.operation were replaced by rpc.service and rpc.method`.

Since that we're still in beta, I believe it's better to remove aws.region for now and we can add it later with a name that has been agreed upon across all SDKs.
Also this change will not be part of this PR since this package isn't producing the shared attributes anymore, it will be added to the Core SDK similar to https://github.com/aws/aws-sdk-net/blob/2cd501b0108ea1c0136094c04c3e1785ff0f0aac/sdk/src/Core/Amazon.Runtime/Telemetry/Tracing/TracingUtilities.cs#L52

github-actions · 2024-08-21T03:16:46Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

muhammad-othman · 2024-08-21T20:30:06Z

Hi @cijothomas,
I've addressed the comments on this PR and the two component owners approved it, please let me know if there are any additional steps needed from my side to merge it.

cijothomas · 2024-08-21T21:56:31Z

There is a CI failure: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/actions/runs/10498004987/job/29082060927?pr=1974 Could you check and fix? I can merge once CI is passing.

…trumentation

muhammad-othman · 2024-08-21T22:40:26Z

Thank you @cijothomas for the quick response.
Looks like exported items were mutated when other tests run at the same time, which caused Collection was modified exception.
I moved the exportItems asserts outside the using clause, this way we are sure the tracer provider was disposed before enumerating over the exportedItems and they shouldn't be modified at this stage.

cijothomas · 2024-08-22T02:42:08Z

@vishweshbankwar Need your help. One of the merge requirement is not met, maybe a CI is missing? I am not able to tell quickly..

muhammad-othman · 2024-08-22T13:07:34Z

@cijothomas I think it needs code owner approver first, someone from open-telemetry/dotnet-contrib-approvers. I believe the previous PR was approved by you then Code owner review required disappeared.

muhammad-othman requested a review from a team July 24, 2024 21:57

github-actions bot assigned ppittle and srprash Jul 24, 2024

github-actions bot requested review from ppittle and srprash July 24, 2024 21:57

github-actions bot added the comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS label Jul 24, 2024

muhammad-othman force-pushed the aws-instrumentation-using-tracerprovider branch from 3f26607 to 6f9c968 Compare July 26, 2024 22:25

muhammad-othman mentioned this pull request Jul 26, 2024

[Instrumentation.AWS] Add AWS metrics instrumentation #1980

Merged

2 tasks

ppittle approved these changes Jul 28, 2024

View reviewed changes

normj suggested changes Aug 1, 2024

View reviewed changes

normj approved these changes Aug 1, 2024

View reviewed changes

martincostello reviewed Aug 9, 2024

View reviewed changes

muhammad-othman added 3 commits August 12, 2024 13:04

[Instrumentation.AWS] Update the instrumentation logic to use AWS Tra…

e88b210

…cerProvider.

Update CHANGELOG

eba2928

Set AWS meter and tracer classes to sealed to help with JIT optimisat…

2cf1ec6

…ions

muhammad-othman force-pushed the aws-instrumentation-using-tracerprovider branch from afc86ad to 2cf1ec6 Compare August 12, 2024 18:10

github-actions bot added the Stale label Aug 21, 2024

srprash approved these changes Aug 21, 2024

View reviewed changes

Merge branch 'main' into aws-instrumentation-using-tracerprovider

f99bf7d

cijothomas closed this Aug 21, 2024

cijothomas reopened this Aug 21, 2024

Move exportItems asserts outside the using clause in TestAWSClientIns…

17c295a

…trumentation

github-actions bot removed the Stale label Aug 22, 2024

cijothomas approved these changes Aug 22, 2024

View reviewed changes

cijothomas merged commit 131ccf3 into open-telemetry:main Aug 22, 2024

muhammad-othman mentioned this pull request Aug 22, 2024

Request to release OpenTelemetry.Instrumentation.AWS package #2018

Merged

4 tasks

muhammad-othman deleted the aws-instrumentation-using-tracerprovider branch August 22, 2024 17:23

muhammad-othman mentioned this pull request Oct 14, 2024

REQUEST: New membership for @muhammad-othman open-telemetry/community#2397

Closed

6 tasks

normj mentioned this pull request Sep 2, 2025

REQUEST: New membership for normj open-telemetry/community#2964

Closed

6 tasks


		namespace OpenTelemetry.Instrumentation.AWS.Implementation.Tracing;

		internal class AWSTraceSpan : TraceSpan

Conversation

muhammad-othman commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

linux-foundation-easycla bot commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kielek commented Jul 25, 2024

Uh oh!

normj commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muhammad-othman commented Aug 1, 2024

Uh oh!

normj commented Aug 1, 2024

Uh oh!

normj Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

muhammad-othman Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normj Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

muhammad-othman Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijothomas commented Aug 5, 2024

Uh oh!

codecov bot commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

martincostello Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

muhammad-othman Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

birojnayak commented Aug 13, 2024

Uh oh!

muhammad-othman commented Aug 13, 2024

Uh oh!

github-actions bot commented Aug 21, 2024

Uh oh!

muhammad-othman commented Aug 21, 2024

Uh oh!

cijothomas commented Aug 21, 2024

Uh oh!

muhammad-othman commented Aug 21, 2024

Uh oh!

cijothomas commented Aug 22, 2024

Uh oh!

muhammad-othman commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

muhammad-othman commented Jul 24, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jul 24, 2024 •

edited

Loading

normj commented Jul 31, 2024 •

edited

Loading

muhammad-othman Aug 1, 2024 •

edited

Loading

muhammad-othman Aug 1, 2024 •

edited

Loading

codecov bot commented Aug 5, 2024 •

edited

Loading