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

Migrate to semconv for otelmux #6638

Open
dmathieu opened this issue Jan 17, 2025 · 5 comments
Open

Migrate to semconv for otelmux #6638

dmathieu opened this issue Jan 17, 2025 · 5 comments

Comments

@dmathieu
Copy link
Member

dmathieu commented Jan 17, 2025

Migrate the otelmux package to generate the semconv package, and use it for generating traces and metrics semantic conventions, like the otelhttp package does.

@MrAlias
Copy link
Contributor

MrAlias commented Jan 24, 2025

@martinyonatann @TanishqPorwar : please coordinate among yourselves here and determine how to unify your efforts. Two separate PRs is not a viable solution.

Please let me know who this issues should be assigned to. Also, please outline your planned solution prior to any future PRs so we can ensure some level of consensus on the approach.

@TanishqPorwar
Copy link
Contributor

@MrAlias Thank you for pointing this out. I had a look at @martinyonatann's PR and noticed that it is quite comprehensive, but it involves implementing separate logic for otelmux. My intent with raising PR (#6653) was to propose an alternate approach that focuses on reusing otelhttp.

This reuse aligns with semconv out of the box and avoids duplicating logic, making it simpler to maintain and ensuring consistency across instrumentations. I thought it would be worth exploring.

@martinyonatann I’d like to understand your approach better to identify any synergies or areas where we can consolidate efforts. I’m happy to adapt or refine my PR based on our discussion.

@dmathieu
Copy link
Member Author

There are two things happening here.
The migration to the semconv package, and refactorings of otelmux itself, to use otelhttp.

The latter is interesting, and makes sense.
The former is what is tracked in this issue, and has some urgency since we want to drop support for old semconv versions.

We should switch otelmux to the semconv package with no new features. Then, we can look into making things better.

@martinyonatann
Copy link
Contributor

martinyonatann commented Jan 27, 2025

I currently have 3 PRs related to this issue:

  1. Setup Body Wrapper Package
  2. Migrate semconv and Implement the Package in Tracing
  3. Migrate semconv and Implement the Package in Metrics

These PRs are modular but interrelated. The body wrapper package (PR 1) is a prerequisite for metrics (PR 3).

I recommend reviewing and merging them sequentially, starting with PR 1, followed by PR 2, and then PR 3.
Please let me know if this approach works or if you’d prefer a different strategy. I'm happy to adjust based on the team's preferences.

@dmathieu
Copy link
Member Author

dmathieu commented Jan 27, 2025

The body wrapper package (PR 1) is a prerequisite for metrics (PR 3).

Metrics is a new feature though. So we can ship semconv without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

No branches or pull requests

4 participants