-
Notifications
You must be signed in to change notification settings - Fork 494
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
Open Telemetry: Adds open telemetry based versioning #4854
base: master
Are you sure you want to change the base?
Conversation
987f979
to
62fc4fb
Compare
27de48e
to
b048737
Compare
/// Factory for handling telemetry trace stability modes, allowing attribute settings | ||
/// based on environment-specified stability mode configurations. | ||
/// </summary> | ||
internal class TracesStabilityFactory |
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.
I get what you want to do here - but why not use subclasses. Like have a CosmosTracer base class (withonly private ctor) and a static factory methode that will create the right subclass (each implementing an abstract populateXXX method). That avoids the need to change signatures when a third version is added and seems more readbale/intuitive than passing around actions?
@@ -18,5 +18,10 @@ internal sealed class OpenTelemetryStablityModes | |||
/// emit both the old and the stable database conventions, allowing for a seamless transition. | |||
/// </summary> | |||
public const string DatabaseDupe = "database/dup"; | |||
|
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.
I still can't say i understasnd/like the version identifiers. That put aside - the docs should refer to a place where the difference (schema of events) is clearly spelled out for the different versions. Can be a separate md file just referenced here.
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.
I agree with you, and I must admit, I'm not a fan of this versioning identifier either. It's somewhat difficult to understand, but we’re aligning with the OpenTelemetry community's decision to maintain consistency across products.
do you mean, maintaining a md
file with an information about, list of attributes getting generated for each stability modes?
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.
LGTM overall but left two comments.
Description
Introducing versioning for open telemetry attributes, to make we are always backward compatible.
database
=> Have latest stable/experimental attributes, In our case it would otel supported attributes.database/dup
=> have latest and old stable/experimental attributes, in our case it would be otel supported attributes and appinsights supported attributes, it is normal to see same value in 2 different attribute keys.default => old attributes. in our case, it is Appinsights Compatible attributes
appinsightssdk
=> reserved for future, when we GA open telemetry attributes, then by default open telemetry compatible attributes will be emitted but customer would be able to get backward compatibility, if they set this env variable. It is not used yet.Other minor changes,
db.cosmosdb.item_count
=>db.cosmosdb.row_count
Code changes Overview
TracesStabilityFactory.cs
: Responsible to take care of version based attributes.Type of change
Please delete options that are not relevant.