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

Identify the first version of semconv that is interpretable by Weaver #181

Closed
lquerel opened this issue May 29, 2024 · 4 comments
Closed
Labels
documentation Improvements or additions to documentation

Comments

@lquerel
Copy link
Contributor

lquerel commented May 29, 2024

Add this information to the documentation.

@lquerel lquerel added the documentation Improvements or additions to documentation label May 29, 2024
@MadVikingGod
Copy link
Contributor

TLDR: v1.19.0-v1.26.0 work.

I explored this a bit and found:
from open-telemetry/semantic-conventions these worked:

from open-telemetry/opentelemetry-specification these work:

v1.18.0 failed with the following error:

/weaver registry resolve -r ../opentelemetry-specification/semantic_conventions -f yaml
⠇ Resolving registry `../opentelemetry-specification/semantic_conventions`Diagnostic report:

  × The semantic convention spec is invalid (path_or_url: "../opentelemetry-specification/semantic_conventions/
  │ trace/http.yaml"). groups[2].attributes: data did not match any variant of untagged enum AttributeSpec at line
  │ 135 column 7


Total execution time: 0.100291173s

I don't know if this is a simple error to bypass.

@lquerel
Copy link
Contributor Author

lquerel commented May 31, 2024

I believe the issue comes from requirement_level: optional used for one of the attributes. This is not a variant supported by Weaver. My experience with SemConv isn't extensive enough to know if this variant was removed or renamed, but that's probably what happened. If necessary, we could add this variant and issue a warning. @MadVikingGod and @jsuereth What do you think?

@MadVikingGod
Copy link
Contributor

I would do an exploration of this. If this is the only problem stopping us from parsing v1.18 and any other before, then yes, I would try to fix it. But if this just uncovers more issues, it's probably not a path worth pursuing.

@MadVikingGod
Copy link
Contributor

With #199 Merged weaver supports v1.13.0 to v1.26.0. I think we can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Done
Development

No branches or pull requests

2 participants