Skip to content
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

telemetry.distro.name and telemetry.distro.version #178

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Jul 7, 2023

  • rename telemetry.auto.name resource attribute telemetry.distro.name
  • add telemetry.distro.version resource attribute

@zeitlinger zeitlinger requested review from a team July 7, 2023 11:32
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Approved, assuming this is already used / prototyped by the Java instrumentation distributions.

@zeitlinger
Copy link
Member Author

No, it's not prototyped yet. We wanted to have feedback from the spec first - I'll do a prototype now 😄

@Kielek
Copy link
Contributor

Kielek commented Jul 28, 2023

Do you have any recommendation how to call official distributions?

There is a strict recommendation what to set for sdk.name attribution

**[1]:** The OpenTelemetry SDK MUST set the `telemetry.sdk.name` attribute to `opentelemetry`.
If another SDK, like a fork or a vendor-provided implementation, is used, this SDK MUST set the
`telemetry.sdk.name` attribute to the fully-qualified class or module name of this SDK's main entry point
or another suitable identifier depending on the language.

@breedx-splk
Copy link
Contributor

I would be in favor of telemetry.distro.name (and version) instead of auto because we also have the need in Android, where it's mostly not "auto-instrumentation". We can use this regardless, it's just less fitting with auto in the name. I think distro is more flexible and applies cleanly to other situations.

@pellared
Copy link
Member

pellared commented Aug 3, 2023

I would be in favor of telemetry.distro.name (and version) instead of auto because we also have the need in Android, where it's mostly not "auto-instrumentation". We can use this regardless, it's just less fitting with auto in the name. I think distro is more flexible and applies cleanly to other situations.

Then probably we should also rename telemetry.auto.version to telemetry.distro.version. While I think it is a good idea as the name can even include "auto" to make it clear that the distribution is an automatic instrumentation I would rather prefer to have it as a separate PR.

Copy link
Member

@AlexanderWert AlexanderWert left a comment

Choose a reason for hiding this comment

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

I like that proposal.

With a strong +1 on #178 (comment).
So, let's add a similar note for telemetry.auto.name.

@AlexanderWert
Copy link
Member

@zeitlinger How do we want to proceed with that?

Do you mind updating the PR to the proposal from above? (updating to telemetry.distro.name + telemetry.distro.version): #178 (comment)

@zeitlinger
Copy link
Member Author

zeitlinger commented Sep 22, 2023

@zeitlinger How do we want to proceed with that?

Do you mind updating the PR to the proposal from above? (updating to telemetry.distro.name + telemetry.distro.version): #178 (comment)

yes, that's a good idea - done 😄

@zeitlinger zeitlinger force-pushed the telemetr_auto branch 3 times, most recently from 12b4f5f to 5a12795 Compare September 22, 2023 14:39
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

re-approving (new name distro)

@zeitlinger can you update the PR title and description to reflect this change?

@zeitlinger zeitlinger changed the title add telemetry.auto.name Rename telemetry.auto.name resource attribute telemetry.distro.name and add telemetry.distro.version resource attribute Sep 22, 2023
@zeitlinger zeitlinger changed the title Rename telemetry.auto.name resource attribute telemetry.distro.name and add telemetry.distro.version resource attribute telemetry.distro.name and telemetry.distro.version Sep 22, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

Looks good. @zeitlinger Please add a new schema file with a schema transformation for this change (like here).

@Kielek
Copy link
Contributor

Kielek commented Sep 25, 2023

Do you have any recommendation how to call official distributions?

There is a strict recommendation what to set for sdk.name attribution

**[1]:** The OpenTelemetry SDK MUST set the `telemetry.sdk.name` attribute to `opentelemetry`.
If another SDK, like a fork or a vendor-provided implementation, is used, this SDK MUST set the
`telemetry.sdk.name` attribute to the fully-qualified class or module name of this SDK's main entry point
or another suitable identifier depending on the language.

I would like to rise this topic once more time before merging.

@jsuereth
Copy link
Contributor

jsuereth commented Oct 2, 2023

@zeitlinger This still needs a telemetry schema file

schemas/1.22.0 Outdated Show resolved Hide resolved
@pyohannes
Copy link
Contributor

@open-telemetry/specs-semconv-maintainers I guess this can be merged?

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

The schema changes should be reflected in https://github.com/open-telemetry/semantic-conventions/blob/main/schema-next.yaml and not in a new file IIRC.

Also, was the comment from @Kielek addressed?

Do you have any recommendation how to call official distributions?
There is a strict recommendation what to set for sdk.name attribution

**[1]:** The OpenTelemetry SDK MUST set the `telemetry.sdk.name` attribute to `opentelemetry`.
If another SDK, like a fork or a vendor-provided implementation, is used, this SDK MUST set the
`telemetry.sdk.name` attribute to the fully-qualified class or module name of this SDK's main entry point
or another suitable identifier depending on the language.

I would like to rise this topic once more time before merging.

@zeitlinger
Copy link
Member Author

@joaopgrassi @Kielek thanks for the feedback 😄

Added a section on naming the official distro & fixed the schema file.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

Giving the broad number of approvals and the latest change (even if minor) I think others need to look at it again.

docs/resource/README.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.