Skip to content

Conversation

rypdal
Copy link
Contributor

@rypdal rypdal commented Jul 31, 2025

Issue #2947: [Instrumentation.AWS] Add cloud.region attribute to AWS SDK Client instrumentation for DynamoDB.

Changes

Set cloud.region tag on Activity for all AWS SDK Clients.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@rypdal rypdal requested a review from a team as a code owner July 31, 2025 13:42
@github-actions github-actions bot added the comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS label Jul 31, 2025
Copy link

codecov bot commented Jul 31, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.59%. Comparing base (7671c02) to head (c9a321b).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...rumentation.AWS/Implementation/AWSServiceHelper.cs 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2969      +/-   ##
==========================================
- Coverage   69.74%   69.59%   -0.16%     
==========================================
  Files         386      413      +27     
  Lines       15917    16184     +267     
==========================================
+ Hits        11102    11263     +161     
- Misses       4815     4921     +106     
Flag Coverage Δ
unittests-Contrib.Shared.Tests 86.25% <ø> (ø)
unittests-Exporter.Geneva 53.33% <ø> (-0.45%) ⬇️
unittests-Exporter.InfluxDB 95.14% <ø> (ø)
unittests-Exporter.Instana 74.86% <ø> (ø)
unittests-Exporter.OneCollector 94.61% <ø> (ø)
unittests-Extensions 90.65% <ø> (ø)
unittests-Extensions.Enrichment 100.00% <ø> (ø)
unittests-Instrumentation.AWS 83.70% <85.71%> (+0.01%) ⬆️
unittests-Instrumentation.AspNet 74.93% <ø> (ø)
unittests-Instrumentation.AspNetCore 70.32% <ø> (ø)
unittests-Instrumentation.Cassandra 23.52% <ø> (?)
unittests-Instrumentation.ConfluentKafka 14.10% <ø> (ø)
unittests-Instrumentation.ElasticsearchClient 80.12% <ø> (ø)
unittests-Instrumentation.EntityFrameworkCore 78.63% <ø> (+17.21%) ⬆️
unittests-Instrumentation.EventCounters 76.36% <ø> (ø)
unittests-Instrumentation.GrpcCore 91.42% <ø> (ø)
unittests-Instrumentation.GrpcNetClient 79.61% <ø> (ø)
unittests-Instrumentation.Hangfire 84.61% <ø> (ø)
unittests-Instrumentation.Http 74.09% <ø> (ø)
unittests-Instrumentation.Owin 88.62% <ø> (ø)
unittests-Instrumentation.Process 100.00% <ø> (ø)
unittests-Instrumentation.Quartz 78.76% <ø> (ø)
unittests-Instrumentation.Runtime 100.00% <ø> (ø)
unittests-Instrumentation.ServiceFabricRemoting 34.54% <ø> (ø)
unittests-Instrumentation.SqlClient 88.50% <ø> (+0.02%) ⬆️
unittests-Instrumentation.StackExchangeRedis 70.81% <ø> (ø)
unittests-Instrumentation.Wcf 78.95% <ø> (ø)
unittests-OpAmp.Client 61.08% <ø> (?)
unittests-PersistentStorage 65.88% <ø> (ø)
unittests-Resources.AWS 75.00% <ø> (ø)
unittests-Resources.Azure 84.56% <ø> (ø)
unittests-Resources.Container 67.34% <ø> (ø)
unittests-Resources.Gcp 71.42% <ø> (ø)
unittests-Resources.Host 73.91% <ø> (ø)
unittests-Resources.OperatingSystem 76.98% <ø> (ø)
unittests-Resources.Process 100.00% <ø> (ø)
unittests-Resources.ProcessRuntime 79.59% <ø> (ø)
unittests-Sampler.AWS 88.25% <ø> (ø)

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

Files with missing lines Coverage Δ
...on.AWS/Implementation/AWSTracingPipelineHandler.cs 86.90% <100.00%> (+0.48%) ⬆️
...rumentation.AWS/Implementation/AWSServiceHelper.cs 97.56% <75.00%> (-1.16%) ⬇️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Kielek Kielek changed the title * [Instrumentation.AWS] added cloud.region attribute to AWS SDK Clien… [Instrumentation.AWS] added cloud.region attribute to DynamoDB Aug 1, 2025
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

Should this tag be applicable for all semantic convention versions you are supporting? Only part of them? Maybe new one should be introduced?

Please add also CHANGELOG.

@rypdal
Copy link
Contributor Author

rypdal commented Aug 1, 2025

Should this tag be applicable for all semantic convention versions you are supporting? Only part of them? Maybe new one should be introduced?

Please add also CHANGELOG.

Judging by this method

Should this tag be applicable for all semantic convention versions you are supporting? Only part of them? Maybe new one should be introduced?

Please add also CHANGELOG.

As far as I see there was the only change in the v1.34.0 related to interpretation of values in cloud.region. But it should be fine. I don't see the reason why it should be supported only in part of semconv.
https://github.com/open-telemetry/semantic-conventions/blob/main/CHANGELOG.md
@Oberon00 : do you see any issue with supporting it in all semantic convention versions ?

@Oberon00
Copy link
Member

Oberon00 commented Aug 1, 2025

@Oberon00 : do you see any issue with supporting it in all semantic convention versions ?

Yes, I think we should be careful with the versions. In older versions cloud.region was only specified to mean the region the telemetry emitter is running on itself. The meaning of a target region is younger than the attribute name. (OTOH, aws.region was also removed completely, so from practical perspective maybe it's fine?)

@rypdal rypdal changed the title [Instrumentation.AWS] added cloud.region attribute to DynamoDB [Instrumentation.AWS] added cloud.region attribute to activity tags. Aug 1, 2025
@rypdal
Copy link
Contributor Author

rypdal commented Aug 1, 2025

Should this tag be applicable for all semantic convention versions you are supporting? Only part of them? Maybe new one should be introduced?

Please add also CHANGELOG.

The change has been added to CHANGELOG.

[Instrumentation.AWS] applied review suggestion (open-telemetry#2947)

Co-authored-by: Christian Neumüller <[email protected]>
@github-actions github-actions bot added the Stale label Aug 12, 2025
@rypdal
Copy link
Contributor Author

rypdal commented Aug 12, 2025

This PR still needs review/approval from @ppittle and @srprash.

@github-actions github-actions bot removed the Stale label Aug 13, 2025
@martincostello
Copy link
Member

As far as we're aware @ppittle no longer works at AWS so I don't think a review from them will be forthcoming.

@rypdal
Copy link
Contributor Author

rypdal commented Aug 19, 2025

As far as we're aware @ppittle no longer works at AWS so I don't think a review from them will be forthcoming.

Thanks for the info, @martincostello. Somehow automation stated that @ppittle or @srprash can be approves. I was a bit confused thinking they are the only approvers.
As far as I see @martincostello and @Kielek can also approve this PR. Do you have more notes for the PR ?

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

FYI I can review and approve, but I can't merge.

Typically the code owners reviews are sought/required for non-trivial changes to functionality for a specific library.

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

If I do not missed anything, cloud.region attribute will be added to all semantic conventions, despite the version.

Based on https://github.com/open-telemetry/semantic-conventions/blob/v1.34.0/docs/cloud-providers/aws-sdk.md
and https://github.com/open-telemetry/semantic-conventions/blob/v1.33.0/docs/cloud-providers/aws-sdk.md 1.34.0 is the very first version recommending it. I think that it potentially is a breaking change for older semantic conventions. As this package has stable release, you should consider adding new semantic convention only, and do not touch the older (if it was not mentioned there).

I hope I do not missed anything in the adding tags process.

Probably this tag can be added only for DynamoDB for older semantic conventions, but it needs to be double checked.

@Kielek
Copy link
Contributor

Kielek commented Aug 19, 2025

As far as we're aware @ppittle no longer works at AWS so I don't think a review from them will be forthcoming.

Thanks for the info, @martincostello. Somehow automation stated that @ppittle or @srprash can be approves. I was a bit confused thinking they are the only approvers. As far as I see @martincostello and @Kielek can also approve this PR. Do you have more notes for the PR ?

Technically, they are. If you have any good contacts in the AWS team, it will be great to make them aware, that packages are abandoned by them.

@martincostello
Copy link
Member

@normj Are you able to help us out here with regards to ownership of these AWS-specific libraries?

[Instrumentation.AWS] applied suggestion: pull request number instead of issue should be used (open-telemetry#2947)

Co-authored-by: Martin Costello <[email protected]>
@normj
Copy link
Contributor

normj commented Aug 19, 2025

I'm reviewing the PR.

@Kielek it is true @ppittle is no longer at AWS. I'm happy to step in take AWS component ownership of the package in the repo. I'm unsure what the process for being added to the component ownership for the AWS packages.

@rypdal
Copy link
Contributor Author

rypdal commented Aug 20, 2025

If I do not missed anything, cloud.region attribute will be added to all semantic conventions, despite the version.

Based on https://github.com/open-telemetry/semantic-conventions/blob/v1.34.0/docs/cloud-providers/aws-sdk.md and https://github.com/open-telemetry/semantic-conventions/blob/v1.33.0/docs/cloud-providers/aws-sdk.md 1.34.0 is the very first version recommending it. I think that it potentially is a breaking change for older semantic conventions. As this package has stable release, you should consider adding new semantic convention only, and do not touch the older (if it was not mentioned there).

I hope I do not missed anything in the adding tags process.

Probably this tag can be added only for DynamoDB for older semantic conventions, but it needs to be double checked.

If I understand the current versioning implementation correctly it's necessary to introduce new class AWSSemanticConventions_V1_34_0 (cloud.region appeared from 1.34). Then extend corresponding enum and return the new class from this method:

private AWSSemanticConventionsBase GetSemanticConventionVersion()

Then it assumes that users initialisation code should be adapted to configure the new SemanticConventionVersion.V1_34_0 because SemanticConventionVersion.V1_28_0 is the default. Also it means that the Latest != V1_29_0 anymore because the latest becomes V1_34_0. This may sound more risky.
Just adding the new attribute cloud.region to all version starting from V1_28_0 doesn't look like a breaking change unless user code set tag with the same name and our code overwrites it. But it sounds like implausible scenario.

@Kielek
Copy link
Contributor

Kielek commented Aug 20, 2025

You might be right that it is not a breaking change. It just violates semantic convention.
As long package was in beta stage, and there were no concept of switching semantic conventions, this kind of changes was fully fine. With the stable packages, and the possibility to choice the sem.conv. version, it should not affect older versions, where the attribute was not known (not at the level of code, but at the level of sem conv.).

BTW if you decide to introduce new sem. conv, it should cover all changes in sem conv. I would suggest to create 1.36/1.37 (if released) and make full compliance check.

I am not in favor of blocking this PR or any other to AWS. I am just trying to show, where you can expect issues from the developers/end-users.

@rypdal
Copy link
Contributor Author

rypdal commented Aug 20, 2025

You might be right that it is not a breaking change. It just violates semantic convention. As long package was in beta stage, and there were no concept of switching semantic conventions, this kind of changes was fully fine. With the stable packages, and the possibility to choice the sem.conv. version, it should not affect older versions, where the attribute was not known (not at the level of code, but at the level of sem conv.).

BTW if you decide to introduce new sem. conv, it should cover all changes in sem conv. I would suggest to create 1.36/1.37 (if released) and make full compliance check.

I am not in favor of blocking this PR or any other to AWS. I am just trying to show, where you can expect issues from the developers/end-users.

Do you mean to introduce the latest sem.conv. 1.36 in code. Then align all attributes (means not only adding the cloud.region) with this version of sem.conv. Then make a full compliance check ? It might be a big overhead in comparison to the initial scope of the PR.

@Kielek
Copy link
Contributor

Kielek commented Aug 20, 2025

Do you mean to introduce the latest sem.conv. 1.36 in code. Then align all attributes (means not only adding the cloud.region) with this version of sem.conv. Then make a full compliance check ? It might be a big overhead in comparison to the initial scope of the PR.

I think that it is the cost of having stable package relaying on non-stable sem. conv..

@rypdal
Copy link
Contributor Author

rypdal commented Aug 20, 2025

Do you mean to introduce the latest sem.conv. 1.36 in code. Then align all attributes (means not only adding the cloud.region) with this version of sem.conv. Then make a full compliance check ? It might be a big overhead in comparison to the initial scope of the PR.

I think that it is the cost of having stable package relaying on non-stable sem. conv..

I understand. If we claim 1.36 then we guarantee it's fully complaint. It means moving from 29 to 36, i.e. 7 versions ahead. This may induce a lot changes with potential bugs. Could we still have some kind of experimental/unstable version before major release of a stable package ?

@Kielek
Copy link
Contributor

Kielek commented Aug 20, 2025

Do you mean to introduce the latest sem.conv. 1.36 in code. Then align all attributes (means not only adding the cloud.region) with this version of sem.conv. Then make a full compliance check ? It might be a big overhead in comparison to the initial scope of the PR.

I think that it is the cost of having stable package relaying on non-stable sem. conv..

I understand. If we claim 1.36 then we guarantee it's fully complaint. It means moving from 29 to 36, i.e. 7 versions ahead. This may induce a lot changes with potential bugs. Could we still have some kind of experimental/unstable version before major release of a stable package ?

Sure! In scope of alpha/beta, we can have even breaking changes (but for sure, we should avoid this).

@rypdal
Copy link
Contributor Author

rypdal commented Aug 21, 2025

Do you mean to introduce the latest sem.conv. 1.36 in code. Then align all attributes (means not only adding the cloud.region) with this version of sem.conv. Then make a full compliance check ? It might be a big overhead in comparison to the initial scope of the PR.

I think that it is the cost of having stable package relaying on non-stable sem. conv..

I understand. If we claim 1.36 then we guarantee it's fully complaint. It means moving from 29 to 36, i.e. 7 versions ahead. This may induce a lot changes with potential bugs. Could we still have some kind of experimental/unstable version before major release of a stable package ?

Sure! In scope of alpha/beta, we can have even breaking changes (but for sure, we should avoid this).

I had a brief look. Unfortunately such a big version difference results in many changes. If you add new attributes implementation effort it may take a long time to finish. It's not only names change, it also getting of new attributes from different not always clear sources.

@alanwest
Copy link
Member

alanwest commented Aug 21, 2025

I had a brief look. Unfortunately such a big version difference results in many changes. If you add new attributes implementation effort it may take a long time to finish. It's not only names change, it also getting of new attributes from different not always clear sources.

This is exactly how AWS decided to design this package. I agree it leads to more work, but AWS perceived it more beneficial to stabilize the package with the trade off of taking on this additional complexity.

The enumeration of versions becomes meaningless if SemanticConventionVersion.V1_28_0 comes to mean "version 1.28 but also with a little bit of 1.34 sprinkled in".

It might be possible to consider adopting only a portion of 1.34 conventions to begin with, but this would have to be carefully considered. Generally speaking, adding new attributes to spans is not considered a breaking change. Adding new attributes to metrics is considered a breaking change, but we're not dealing with metrics here.

So for example, it might be possible to add SemanticConventionVersion.V1_34_0 and only add cloud.region for now. In follow up work additional things new to 1.34 may be able to be added without being considered a breaking change. Also, I would consider implementing SemanticConventionVersion.V1_36_0 since that is the latest rather than SemanticConventionVersion.V1_34_0. I don't personally see a reason for the enumeration to support all the versions.

Then it assumes that users initialisation code should be adapted to configure the new SemanticConventionVersion.V1_34_0 because SemanticConventionVersion.V1_28_0 is the default.

Yes, this is the intent. It would be a breaking change to change the default. Consumers would need to explicitly opt into 1.34.

Also it means that the Latest != V1_29_0 anymore because the latest becomes V1_34_0. This may sound more risky.

This is also the intent. The idea was that consumers choosing Latest are inherently taking on the risk that the telemetry produced by the library may change as they upgrade to a new version.

Just adding the new attribute cloud.region to all version starting from V1_28_0 doesn't look like a breaking change unless user code set tag with the same name and our code overwrites it. But it sounds like implausible scenario.

It may not technically be a breaking change today, but it could lead to a breaking change later. The AWS SDK conventions are still under development. If the meaning or name of cloud.region were to later change then making that change could be breaking. The idea of introducing the enumeration of versions gives consumers selecting a specific version a guarantee that there will be no breaking changes.

@rypdal
Copy link
Contributor Author

rypdal commented Aug 26, 2025

@alanwest thanks for your explanation.

@rypdal
Copy link
Contributor Author

rypdal commented Aug 26, 2025

I would put this PR on hold (not sure if it's possible), or even close it and re-open later. Perhaps it make sense to collect a portion of 1.34 (or 1.36) convention attributes and then merge them together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants