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

New semconvgen template and update to semconv 1.25.0 #3586

Merged
merged 26 commits into from
May 3, 2024

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Dec 14, 2023

First stab at open-telemetry/semantic-conventions#551

Remaining discussions:

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@lmolkova lmolkova force-pushed the protype-new-codegen branch from 5a5a24e to 9176ade Compare December 14, 2023 04:10
scripts/semconv/generate.sh Outdated Show resolved Hide resolved
scripts/semconv/generate.sh Outdated Show resolved Hide resolved
@lmolkova
Copy link
Contributor Author

@open-telemetry/python-maintainers could you please take another look and tell if there you see any showstopping issues we need to resolve before moving forward with build-tools changes?

@lzchen
Copy link
Contributor

lzchen commented Feb 29, 2024

@lmolkova

Approach looks good to me for the new build-tools.

@ocelotl @aabmass

Could you PTAL and see if you are okay with these changes?

@aabmass
Copy link
Member

aabmass commented Feb 29, 2024

@lmolkova it looks like we decided to go ahead with the version namespace approach, is that right? I have no issue with it but wanted to understand what happens at the next semconv version bump, does v1_23_1 directory stay there or deleted?

@lmolkova lmolkova force-pushed the protype-new-codegen branch from 7d1bb68 to bc5bb60 Compare March 1, 2024 00:36
@lmolkova
Copy link
Contributor Author

lmolkova commented Mar 1, 2024

Discussed offline with @lzchen wrt folder structure.

There was an idea from .NET SIG to generate namespace per-version. I.e. users import v1_23_1.http_attributes and will be able to use attributes from this namespace even when semconv library is updated and now contains v1_24_0, etc.
While it's good for stability, it's blows up artifact size.

As a middle ground, I changed folder structure to

  • opentelemetry-semantic-conventions
    • src\opentelemetry\semconv
      • experimental (attributes, can also move them all to attributes folder)
        • db_attributes.py - individual experimental attribute files go here
      • http_attributes.py, etc - individual stable attribute files go here
      • metrics
        • http_metrics.py - stable metrics go here
        • experimental
          • db_metrics.py - experimental metrics go here
      • events (nothing here yet, but eventually will be)
        • stable events
        • experimental
          • experimental events

The folder structure is fully in Python SIG hands and does not need any build-tools work to be customized.

@lmolkova
Copy link
Contributor Author

lmolkova commented Mar 1, 2024

Documenting options to split stable/experimental

Option 1: ship 1 artifact containing stable and experimental semconv. Emphasize that some parts are experimental via namespace (import semconv.experimental.foo_attributes, perhaps we should call it _experimental )?

Option 2: ship 1 artifact, but two different versions: stable and preview. Stable contains only stable semconv. Preview contains all of them.

  • Pros:
    • ideal in terms of semantic versioning
    • when attribute stabilizes, it remains in the same namespace and all libs/apps that depend on it don't need to change
  • Cons:
    • version resolution would pick the highest version (1.1.0 stable vs 1.0.0-alpha) resulting in runtime issues.

Option 3: ship 2 artifacts (semconv and semconv-experimental) - Java does it this way and uses incubating term instead of experimental

  • Pros:
    • good in terms of semantic versioning
    • easy to maintain
  • Cons:
    • when attribute stabilizes, all libs/apps that depend on it need to change.

The common mitigation to "when attribute stabilizes, all libs/apps that depend on it need to change" could be to preserve stabilized attributes in experimental, incubating namespaces (might need better name for it)


Update: reached consensus with Java SIG

Ship 2 artifacts: semconv (1.0.0 stable) and semconv-incubating (1.0.0-alpha):

  • stable one contains all the stable things
  • experimental one contains everything
  • when attribute is deprecated (we won't remove them from semconv anymore):
    • it stays in the artifact (semconv it was stable, incubating if it was experimental)
    • it's deprecated in the code
  • when attribute stabilizes
    • it appears in the semconv artifact
    • it stays in the incubating artifacts as deprecated pointing to the new location in the stable artifact

The documentation will recommend:

  • lib owners to never depend on incubating artifact, hardcode experimental attributes instead

@lmolkova
Copy link
Contributor Author

lmolkova commented Mar 1, 2024

Hi @open-telemetry/python-maintainers and @open-telemetry/python-approvers, I addressed your feedback.

And here's the summary of remaining decisions I'd like to get your input on

Note: none of this discussions are related to build-tools/jinja templates, so I think it means we can move forward with build-tools changes independently from the results of these discussions. LMK if you have any objections.

@lmolkova
Copy link
Contributor Author

Based on the SIG discussion 3/14, we have the following preliminary consensus on artifacts+stability outlined in #3586 (comment) as Option 3:

  • have 2 artifacts (semantic-conventions and semantic-conventions-experimental|incubating). Different names and import path makes incubation/experimental status obvious
  • Attributes (experimental or stable) are never removed from the incubation artifact
  • Stabilized attributes are deprecated in the incubating artifact an point to stable one

This makes Python consistent with Java approach.

Overall, we have proved that new codegeneration changes provide the flexibility to achieve it and generate idiomatic python code, so we can go ahead with them.

@lmolkova lmolkova force-pushed the protype-new-codegen branch from 81a02da to 619dd0c Compare April 18, 2024 05:49
@lmolkova lmolkova marked this pull request as ready for review April 18, 2024 21:32
@lmolkova
Copy link
Contributor Author

lmolkova commented May 1, 2024

@xrmx

Is there a reason comments are added after and not before the variables?

it seems to be the pattern, e.g. here

OTEL_SDK_DISABLED = "OTEL_SDK_DISABLED"
"""
.. envvar:: OTEL_SDK_DISABLED
The :envvar:`OTEL_SDK_DISABLED` environment variable disables the SDK for all signals
Default: "false"
"""

Happy to change it otherwise.

@lmolkova lmolkova force-pushed the protype-new-codegen branch from 23f6797 to f7a273a Compare May 1, 2024 19:21
@xrmx
Copy link
Contributor

xrmx commented May 2, 2024

@xrmx

Is there a reason comments are added after and not before the variables?

it seems to be the pattern, e.g. here

OTEL_SDK_DISABLED = "OTEL_SDK_DISABLED"
"""
.. envvar:: OTEL_SDK_DISABLED
The :envvar:`OTEL_SDK_DISABLED` environment variable disables the SDK for all signals
Default: "false"
"""

Happy to change it otherwise.

Don't mind, was just curious. I can see it making sense in sphinx documents but feels strange on python code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants