-
Notifications
You must be signed in to change notification settings - Fork 0
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
PREVIEW: Upgrade semantic convention #2
Conversation
0acf686
to
fe78fd3
Compare
160756d
to
5d4f959
Compare
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.
Please add a test to ensure switch across different semantic conventions version works and default semantic version also work.
Thank you for iterating on feedback offline PJ.
src/OpenTelemetry.Instrumentation.AWS/Implementation/AWSServiceHelper.cs
Show resolved
Hide resolved
<Compile Include="$(RepoRoot)\src\Shared\AssemblyVersionExtensions.cs" Link="Includes\AssemblyVersionExtensions.cs" /> | ||
<Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" /> | ||
<Compile Include="$(RepoRoot)\src\Shared\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="..\OpenTelemetry.SemanticConventions\OpenTelemetry.SemanticConventions.csproj" /> |
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.
Can we take a dependency on version (NUGET) rather than project ?
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.
The dependency is only for documentation on internal classes. I want to ask the OTel org the best way to to do this, there's pro's and con's either way.
src/OpenTelemetry.Instrumentation.AWSLambda/Implementation/AWSLambdaHttpUtils.cs
Show resolved
Hide resolved
src/OpenTelemetry.Instrumentation.AWSLambda/Implementation/AWSLambdaResourceDetector.cs
Show resolved
Hide resolved
/// <para> | ||
/// Collection of the Open Telemetry Semantic Conventions supported by the OpenTelemetry.*.AWS libraries. | ||
/// Can be used to pin the version of Semantic Convention emitted. | ||
/// </para> | ||
/// <para> | ||
/// While these libraries are intended for production use, they rely on several Semantic Conventions that are | ||
/// still considered Experimental, meaning they may undergo additional changes before becoming Stable. This can | ||
/// impact the aggregation and analysis of telemetry signals in environments with multiple applications or microservices. | ||
/// For example, a microservice using an older version of the Semantic Conventions for Http Attributes may emit | ||
/// <c>"http.method"</c> with a value of GET,while a different microservice, using a new version of Semantic Convention may instead emit the GET as | ||
/// <c>"http.request.method"</c>. | ||
/// </para> | ||
/// <para> | ||
/// Future versions the OpenTelemetry.*.AWS libraries will include updates to the Semantic Convention, which may break compatibility with a previous version. | ||
/// To opt-out of automatic upgrades, you can pin to a specific version: | ||
/// <code> | ||
/// <![CDATA[ | ||
/// using (var tracerProvider = Sdk.CreateTracerProviderBuilder() | ||
/// .AddAWSLambdaConfigurations(opt => | ||
/// { | ||
/// opt.SemanticConventionVersion = SemanticConventionVersion.v1_10_EXPERIMENTAL; | ||
/// }) | ||
/// .Build()!) |
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.
may be move to readme
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.
good point - I think I want to keep it here, but this information should also be in the readme.md files
5d4f959
to
e70584e
Compare
Added unit test. |
5b1038f
to
853462a
Compare
var resourceAttributes = | ||
new List<KeyValuePair<string, object>>() | ||
.AddAttributeCloudProviderIsAWS() | ||
.AddAttributeCloudPlatformIsAwsEcs(); | ||
try | ||
{ | ||
var containerId = GetECSContainerId(AWSECSMetadataPath); |
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.
Not new to your PR but the work to get the container id seems expensive to do for every request when the container id can't change within a process. This might be true for other properties that requiring going to the metadata but are immutable for the environment.
|
||
var attributeName = attributeNameFunc(semanticConventionVersionImpl); | ||
|
||
activity?.SetTag(attributeName, value); |
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.
Don't you need to check if attributeName
is null or empty?
private class AWSSemanticConventions_v1_10_1 : AWSSemanticConventions_v1_10 | ||
{ | ||
// AWS Attributes | ||
public override string AttributeAWSBedrock => "aws.bedrock"; |
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.
Is the pattern forcing us to create a new SemanticConventions every time we add a new service? Seems like if we are being additive that shouldn't require us to define a new semantic conventions version.
/// Pin to the specific state of all Semantic Conventions as of the 1.10 release of this library. | ||
/// https://github.com/open-telemetry/semantic-conventions/releases/tag/v1.27.0. | ||
/// </summary> | ||
v1_10_Experimental = 1, |
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.
Do the enums need the Experimental
suffix? Seems like the version defines what the convention is and isn't going to change.
0262b7e
to
94d516a
Compare
…semantic convention versions
…ire some additional changes in the Instrumentation.AWSLambda project to accomidate using url.path and url.query instead.
94d516a
to
da63599
Compare
Fixes #.
Changes
PREVIEW: Implementation of the design discussed in the Nov 26th OpenTelemetry .NET SIG meeting. This design allows releasing the AWS OTel Contrib libraries as GA while still informing users that underlying Semantic Conventions are experimental.
This is exposed via the
SemanticConventionVersion
enum:Users can choose how to handle this for their applications by either pinning to a specific Semantic Convention version or automatically getting updates the next time they upgrade the AWS Otel nuget packages.
Selection is made by updating the Options object in the builder method.