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

Mark service and telemetry.sdk resource attributes as stable. #3202

Merged
merged 17 commits into from
Apr 4, 2023

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Feb 10, 2023

Changes

This PR marks the set of resource semantic conventions, which a relied upon in stable API / SDK specification, as stable themselves.

  • Add a caveat that changing service.* and telemetry.* names need MORE than just schema file changes.
  • Mark core Resource attribute semconv document as MIXED stability
  • Update key sections for Resource attribute semconv as stable
  • Mark telemetry.sdk and service attribute groups as stable.
  • Move telemetry.auto and service.* "contentious" attributes into {section}_experimental.yaml file. This retains the reserved name, but does not mark the convention as stable, allowing us to ensure stable portions of our specification refer to stable semconv.
  • Update examples for service.instance.id to include an example of the preferred style instance id before the fallback.

Related issues #3177

Usage in the Specification

One reason we should mark these attributes as stable is because they are referenced from numerous stable portions of our specification, and they are also leveraged from newly stabilizing portions of our specification.

A note on ECS compatibility

Here's ECS service definition.

They include more attributes than we do. The only possible conflict between us is service.instance.id vs. service.node.name. In this case, our service.instance.id has a fallback of generating UUID which is inconsistent w/ service.node.name which requires a user-generated name if another name cannot be provided.

There's an opportunity for us to unify here. However, I don't think there's appetite in OTEL to require user-specified names in lieu of some algorithm or generative specification.

@jsuereth jsuereth requested review from a team February 10, 2023 18:21
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with a changelog entry.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Feb 10, 2023

I believe we need to explicitly specify what declaring a semantic convention stable means.

There are 2 possibilities:

  1. We follow the current definition which says that changes that can be described by Schema files are allowed and additive changes are allowed. I am personally in favour of this approach.
  2. Instead, because Schema Files are not yet fully implemented and are not yet honoured by many components in Otel we aim for stronger (more restrictive) stability guarantees, i.e. only additive changes are allowed. Once Schema Files are widely honoured we can lift the unnecessary restrictions and allow Schema File-based changes too. I do not think this is the right approach but some people may expect "Stable" to mean this so we need to explicitly say that "no this is not what declaring stable means".

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory labels Feb 14, 2023
@carlosalberto
Copy link
Contributor

Instead, because Schema Files are not yet fully implemented and are not yet honoured by many components in Otel we aim for stronger (more restrictive) stability guarantees, i.e. only additive changes are allowed.

I'd go with this (conservative) option as, precisely, full Schema Files support aren't complete yet. It may surprise users to know an X or Y semantic convention is stable and then we can't provide massaging for any unexpected change.

@tsloughter
Copy link
Member

I don't see this mentioned, I'd delay this until #3210 is resolved. Maybe can be figured out today in the spec sig meeting.

@jsuereth
Copy link
Contributor Author

@tsloughter Regarding #3210 - I think the same thing I said about service.instance.id holds here. The existence of service.name and its meaning is not changing. We should be able to stabilize that, while having an experimental provision for how SDKs are required to generate the value.

@jsuereth
Copy link
Contributor Author

Update this so that it fragments service and telemetry resource attributes into stable + experimental components.

@reyang @dashpole @carlosalberto PTAL as this may alter your approvals.

@Oberon00 @arminru I've updated based on your suggestion to fragment stable/experimental portions. PTAL

Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

With splitting it up and only marking service.name and the three telemetry.sdk.* attributes stable in the first iteration I think this is good to go. 👍

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 3, 2023
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 11, 2023
@jsuereth jsuereth reopened this Mar 13, 2023
@carlosalberto
Copy link
Contributor

@tsloughter Any last concern? The related issue seems to have discussed and found general approval on resource detectors as a solution for that problem, which would make this PR ready to go in.

@tsloughter
Copy link
Member

@carlosalberto no concern from me anymore 👍

@github-actions github-actions bot removed the Stale label Mar 21, 2023
@jmacd
Copy link
Contributor

jmacd commented Mar 27, 2023

@jsuereth is this alive?

@jsuereth
Copy link
Contributor Author

@jmacd Yes.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor question that may be worth clarifying.

specification/versioning-and-stability.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.