-
Notifications
You must be signed in to change notification settings - Fork 899
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
Add semantic convention for telemetry.auto.version #799
Add semantic convention for telemetry.auto.version #799
Conversation
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
@@ -80,6 +80,7 @@ The identifier SHOULD be stable across different versions of an implementation. | |||
| telemetry.sdk.name | The name of the telemetry SDK as defined above. | `opentelemetry` | No | | |||
| telemetry.sdk.language | The language of the telemetry SDK.<br/> One of the following values MUST be used, if one applies: "cpp", "dotnet", "erlang", "go", "java", "nodejs", "php", "python", "ruby", "webjs" | `java` | No | | |||
| telemetry.sdk.version | The version string of the telemetry SDK as defined in [Version Attributes](#version-attributes). | `semver:1.2.3` | No | | |||
| telemetry.auto.version | The version string of the auto instrumentation agent, if used, as defined in [Version Attributes](#version-attributes). | `semver:1.2.3` | No | |
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 be resource-wide or specific library-wide? Are we expecting mix and match of auto and non-auto telemetry in a single resource?
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.
This is resource-wide to report version of the auto instrumentation used in the given process. Library/trace-specific versions, which may differ from this version, are available as part of named Tracer/Meter
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 a strong block. I think this attribute is very much java-auto-instr inspired. Would it be different in case of other languages where we might have auto and non-auto telemetry in a single resource? Or we will use separate resources in this situation?
@SergeyKanzhelev I don't understand your concern about it being java-specific. Every language that uses auto instrumentation (.NET, js, ruby?) will probably end up in a situation when auto-instrumentation release version differs (if only in minor/patch parts) from the SDK version. In general, we have up to 3 versions in play at every given time:
|
cc @codeboten |
I agree w/ @iNikem that this is not java specific. In the Python implementation, the |
@SergeyKanzhelev do you still want to block this PR |
Sorry, wasn't very clear. My question is whether auto.version belongs to resource or not? What if resource sends telemetry via both - manual instrumentation and auto instrumentation. Should both carry the auto.version as resource attributes? Or it will be library version? Or those would be separate resources? When I'm saying java specific, I mean that in Java there is not really manual instrumetnaiton when java agent present. java agent hijecks all instrumentation and adding auto.version to the resource is very logical |
Just a clarification, when the javaagent is present, there still is manual instrumentation. It does replaces the SDK though, if one was brought by the user. |
In addition to this every span will have library info, which name will indicate the exact instrumentation library that has produced this span. Manual and auto instrumentation, I would assume, will use different names to get |
I agree that putting auto version into Library may be something we can decide to do in future. But this PR is not blocking us from doing it. |
@carlosalberto can it be merged now? |
no changes in spec for 2 working days and enough approvals. Merging |
Changes
In addition to the version of the SDK, I propose to add a resource semantic convention for the optional version of the auto instrumentation agent if that is used.