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

Add frozen stability level and change default stability level to experimental #170

Closed
wants to merge 4 commits into from

Conversation

lmolkova
Copy link
Contributor

In preparation for open-telemetry/opentelemetry-specification#3279, it'd be great to have a formal way to declare attributes as frozen.

@lmolkova lmolkova requested a review from a team April 25, 2023 22:35
@lmolkova lmolkova requested a review from Oberon00 as a code owner April 25, 2023 22:35
@lmolkova
Copy link
Contributor Author

@Oberon00 @arminru could you please take a look? Thanks!

@@ -200,6 +201,12 @@ def add_md_parser(subparsers):
required=False,
action="store_true",
)
parser.add_argument(
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this? I think the default would be to extend the "brief", and this would be just some extra-prominent formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that when an attribute becomes frozen, we change it's brief to mention it there? It's a viable solution, but then it's harder to read and review, harder to follow the same pattern, and easier to overlook.
It's also not possible to add any checks (if we want to).

I also think it could be useful to add a stability column and represent stability information separately from brief.

@@ -314,6 +315,7 @@ def check_stability(stability_value, position):
"deprecated": StabilityLevel.DEPRECATED,
"experimental": StabilityLevel.EXPERIMENTAL,
"stable": StabilityLevel.STABLE,
"frozen": StabilityLevel.FROZEN,
Copy link
Member

@Oberon00 Oberon00 Apr 27, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@lmolkova lmolkova Apr 27, 2023

Choose a reason for hiding this comment

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

trying to understand the concern: is it a different term or you'd rather see it as a separate flag?

  1. If it's a term, we chatted with @reyang and @trask about frozen vs feature-freeze and considered (in the long term) changing the feature-freeze to frozen to apply better to things like attributes and be an adjective following stable or deprecated pattern. Go uses Frozen convention for packages to describe that no new features are accepted.
  2. If you'd rather see it as a separate flag: feature-freeze is still a valid document status used interchangeably with Stable or Experimental. Having two attribute parameters would be confusing, as I can write something meaningless as
stability: stable | deprecated
feature-freeze: false

Copy link
Member

Choose a reason for hiding this comment

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

2 is what I mean. As you can see in the linked document, a spec document has a lifecylce status and it can be feature-frozen or not. So indeed:

stability: stable | deprecated | experimental(default)
feature-freeze: true | false(default)

At least I would suggest adopting the same system for whole spec documents and semantic convention elements, otherwise it could get confusing.

I have no strong opinion on whether that is ideal or if "frozen" should actually be a lifecylce status (optional step between experimental and stable?), but then I think it should be changed in the specs document-status spec first (which itself doesn't state a document status which would make it experimental, whether intentional or not)

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/document-status.md

Copy link
Member

Choose a reason for hiding this comment

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

@dyladan
Copy link
Member

dyladan commented Jul 19, 2023

I was updating the JS semconv package and was very surprised to see the default stability level is stable. We're trying to publish stable separately from experimental in preparation for semconv like HTTP becoming stable soon. Is there any way we can move this forward or at least change the default to experimental?

@arminru
Copy link
Member

arminru commented Jul 24, 2023

@dyladan I carved the default part out into #189 since the discussions on open-telemetry/opentelemetry-specification#3456 might still take some time to resolve.

@arminru arminru requested a review from a team July 24, 2023 17:24
@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 6, 2023

Closing this until we have conclusion on open-telemetry/opentelemetry-specification#3459 and open-telemetry/oteps#232

@lmolkova lmolkova closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants