-
Notifications
You must be signed in to change notification settings - Fork 345
[Instrumentation.AWS] added cloud.region attribute to activity tags. #2969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[Instrumentation.AWS] added cloud.region attribute to activity tags. #2969
Conversation
…t instrumentation for DynamoDB (open-telemetry#2947)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
test/OpenTelemetry.Instrumentation.AWS.Tests/OpenTelemetry.Instrumentation.AWS.Tests.csproj
Outdated
Show resolved
Hide resolved
…mock of IRequestContext (open-telemetry#2947)
src/OpenTelemetry.Instrumentation.AWS/Implementation/AWSServiceHelper.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
test/OpenTelemetry.Instrumentation.AWS.Tests/Implementation/AWSServiceHelperTests.cs
Outdated
Show resolved
Hide resolved
Judging by this method
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. |
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?) |
The change has been added to CHANGELOG. |
test/OpenTelemetry.Instrumentation.AWS.Tests/Implementation/AWSServiceHelperTests.cs
Outdated
Show resolved
Hide resolved
[Instrumentation.AWS] applied review suggestion (open-telemetry#2947) Co-authored-by: Christian Neumüller <[email protected]>
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. |
There was a problem hiding this 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.
test/OpenTelemetry.Instrumentation.AWS.Tests/Implementation/AWSServiceHelperTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.AWS.Tests/Implementation/AWSServiceHelperTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.AWS.Tests/Implementation/AWSServiceHelperTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Instrumentation.AWS.Tests/Tools/MockRequestContext.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
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. |
@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]>
…//github.com/rypdal/opentelemetry-dotnet-contrib into feature/instrumentation-aws-add-cloud-region
If I understand the current versioning implementation correctly it's necessary to introduce new class AWSSemanticConventions_V1_34_0 (
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.
|
You might be right that it is not a breaking change. It just violates semantic convention. 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. |
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. |
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 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
Yes, this is the intent. It would be a breaking change to change the default. Consumers would need to explicitly opt into 1.34.
This is also the intent. The idea was that consumers choosing
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 |
@alanwest thanks for your explanation. |
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. |
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
CHANGELOG.md
files updated for non-trivial changes