opentelemetry-python icon indicating copy to clipboard operation
opentelemetry-python copied to clipboard

New semconvgen prototype

Open lmolkova opened this issue 1 year ago • 6 comments

First stab at https://github.com/open-telemetry/semantic-conventions/issues/551

Remaining discussions:

  • [ ] How to generate metrics. See https://github.com/open-telemetry/opentelemetry-python/pull/3586#discussion_r1509494281 for options
  • [ ] How to separate stable semconv from experimental. See https://github.com/open-telemetry/opentelemetry-python/pull/3586#issuecomment-1973978521 for options

Please delete options that are not relevant.

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

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

  • [x] No.

Checklist:

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

lmolkova avatar Dec 14 '23 03:12 lmolkova

@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?

lmolkova avatar Feb 28 '24 21:02 lmolkova

@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?

lzchen avatar Feb 29 '24 18:02 lzchen

@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?

aabmass avatar Feb 29 '24 21:02 aabmass

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 avatar Mar 01 '24 21:03 lmolkova

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 )?

  • Pros: easy to maintain
  • Cons:
    • violates semantic versioning
    • when attribute stabilizes, it needs to be moved, i.e. libs/apps that depend on it need to change .See https://github.com/open-telemetry/semantic-conventions-java/pull/45#issuecomment-1973934974

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 avatar Mar 01 '24 21:03 lmolkova

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

  • [ ] How to generate metrics. See https://github.com/open-telemetry/opentelemetry-python/pull/3586#discussion_r1509494281 for options
  • [ ] How to separate stable semconv from experimental. See https://github.com/open-telemetry/opentelemetry-python/pull/3586#issuecomment-1973978521 for options - Based on it we can also finalize the folder structure. Current structure is described here https://github.com/open-telemetry/opentelemetry-python/pull/3586#issuecomment-1973958604

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 avatar Mar 01 '24 22:03 lmolkova

Based on the SIG discussion 3/14, we have the following preliminary consensus on artifacts+stability outlined in https://github.com/open-telemetry/opentelemetry-python/pull/3586#issuecomment-1973978521 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 avatar Mar 14 '24 18:03 lmolkova

@xrmx

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

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

https://github.com/open-telemetry/opentelemetry-python/blob/d285b7f6f51b5b2f28debc072d997726e67f3574/opentelemetry-sdk/src/opentelemetry/sdk/environment_variables.py#L15-L21

Happy to change it otherwise.

lmolkova avatar May 01 '24 18:05 lmolkova

@xrmx

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

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

https://github.com/open-telemetry/opentelemetry-python/blob/d285b7f6f51b5b2f28debc072d997726e67f3574/opentelemetry-sdk/src/opentelemetry/sdk/environment_variables.py#L15-L21

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.

xrmx avatar May 02 '24 09:05 xrmx