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

Exclude attributes without stability from stable semconv part #4391

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Jan 22, 2025

See open-telemetry/semantic-conventions#1777

Semconv should not have attributes without stability, but this one slipped in.
We'll fix it in semconv and also enforce it better to prevent this in the future - open-telemetry/semantic-conventions#1781, but let's exclude attributes without stability from the stable part.

Since semconv artifact is at 0.50b0, I assume it's allowed to have breaking changes?
This attribute is not used by anything in OTel python, was introduced in prev version of semconv and I'm suggesting to drop it.

Type of change

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

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

@lmolkova lmolkova requested a review from a team as a code owner January 22, 2025 03:59
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@aabmass
Copy link
Member

aabmass commented Jan 23, 2025

Since semconv artifact is at 0.50b0, I assume it's allowed to have breaking changes?

Ya it should be fine. We were discussing and getting serious about going to a stable version though 😥 Are other languages special casing this to keep it around and not introduce a breakage?

@lzchen
Copy link
Contributor

lzchen commented Jan 23, 2025

@lmolkova

I think instances like this make us wary for making semantic convention packages stable (which as @aabmass mentioned above we were thinking of doing since we have stable/incubating namespaces.

Are other languages special casing this to keep it around and not introduce a breakage?

If there is a good process for special cases like this to prevent breaking changes it might give us some more confidence to go stable :)

@lmolkova
Copy link
Contributor Author

lmolkova commented Jan 24, 2025

I understand your concerns. It should not have happened.

Are other languages special casing this to keep it around and not introduce a breakage?

It was caught by JS during update, and only a few languages got affected. Java has additional filters that prevented.
We'll have a more reliable filter in weaver - open-telemetry/weaver#569 for this.

It hit c++ the worst since semconv are part of stable otel api package - open-telemetry/opentelemetry-cpp#3251


I think instances like this make us wary for making semantic convention packages stable (which as @aabmass mentioned above we were thinking of doing since we have stable/incubating namespaces.

Java goes stable with semantic convention (stable) artifact.

We fixed the problem in semconv - stability is now required. And this specific issue should not happen again.

From python side, we should review public API more carefully in a stable artifact. Of course small unintentional additions could slip in by mistake. I hope it won't happen again, but if it does, we can still use an escape hatch like this one open-telemetry/opentelemetry-cpp@main...lmolkova:opentelemetry-cpp:workaround-network-interface-name (or what we've done in the past keeping removed attributes hardcoded in jinja templates)


So I think if you want to release stable semconv, we should make sure we have public API checks in place and in the worst case, if something extra gets in, we'd have to keep it around. Let me know if you have any thoughts on how we can harden it further. I'll also bring it up in semconv call on Monday.

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.

6 participants