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

[draft] Migrate to semconv for otelmux #6638 #6653

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TanishqPorwar
Copy link
Contributor

tried using otelhttp.Handler in otelmux to address #6638

@github-actions github-actions bot requested a review from akats7 January 21, 2025 04:28
@TanishqPorwar
Copy link
Contributor Author

@dmathieu would it make sense to use otelhttp since it already adheres to semconv? I'll tidy up this PR if this approach aligns with the right direction.

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 89.13043% with 5 lines in your changes missing coverage. Please review.

Project coverage is 68.7%. Comparing base (9a53a30) to head (92636cd).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...mentation/github.com/gorilla/mux/otelmux/config.go 0.0% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6653     +/-   ##
=======================================
- Coverage   68.7%   68.7%   -0.1%     
=======================================
  Files        200     200             
  Lines      16872   16864      -8     
=======================================
- Hits       11601   11590     -11     
- Misses      4927    4932      +5     
+ Partials     344     342      -2     
Files with missing lines Coverage Δ
...trumentation/github.com/gorilla/mux/otelmux/mux.go 93.5% <100.0%> (+4.3%) ⬆️
...mentation/github.com/gorilla/mux/otelmux/config.go 84.8% <0.0%> (-15.2%) ⬇️

... and 5 files with indirect coverage changes

@dmathieu
Copy link
Member

Yes, if we can get HTTP instrumentations to use otelhttp rather than reimplement things themselves, that's definitely better.

@akats7
Copy link
Contributor

akats7 commented Jan 21, 2025

Yes, if we can get HTTP instrumentations to use otelhttp rather than reimplement things themselves, that's definitely better.

@dmathieu wasn't there talk of refactoring otelhttp, not sure to what extent the intended changes would be breaking

@dmathieu
Copy link
Member

dmathieu commented Jan 21, 2025

Yes. But if/when that happens, we will make the appropriate changes to avoid breaking other instrumentations. Reusing otelhttp also makes it much easier to keep. things working the same way.

We won't be able to make breaking changes in otelhttp that easily though, because many folks rely on it as a stable dependency despite it not being one.

@dmathieu
Copy link
Member

@TanishqPorwar did you see you're working on this alongside @martinyonatann (#6648)?

@TanishqPorwar
Copy link
Contributor Author

@dmathieu thanks for pointing this out. My goal with this PR was to propose an alternate solution by reusing otelhttp.

This approach minimizes redundancy, adheres to semconv, and simplifies future maintenance. That said, I’m open to discussing how we can align our efforts. @martinyonatann, let me know your thoughts, and we can figure out the best way forward.

@martinyonatann
Copy link

from my personal perspective, to avoid unintended changes to otelhtttp, i prefer to reimplement the semconv and request packages because that aligns with the purpose of creating the package template.

Reusing otelhttp is definitely is nice-to-have, and i agree that it's a goal worth pursuing. However, we'll need to carefully pan how we can reuse it across frameworks that are built on net/http, such as mux, echo, gin, etc.

Each framework has its nuances, so ensuring a consistent implementation while accommodating these differences is key. Let's discuss how we can create a roadmap or strategy for this, so we can make the transition as smooth and efficient as possible.

@akats7
Copy link
Contributor

akats7 commented Jan 25, 2025

I think for this current iteration and to first complete the semconv migration let’s go with the reimplementation, especially since a good amount of work has been completed. We can continue to evaluate the merits of reusing otelhttp but I agree there are additional considerations we need to make if we’re going to start using otelhttp as a base.

@dmathieu
Copy link
Member

Let's keep the discussion in a single place, #6638.

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.

4 participants