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

feat: support metrics for otelmux #6648

Conversation

martinyonatann
Copy link
Contributor

Address issue #6638

@martinyonatann martinyonatann requested a review from a team as a code owner January 19, 2025 18:39
Copy link

linux-foundation-easycla bot commented Jan 19, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from akats7 January 19, 2025 18:39
@dmathieu
Copy link
Member

Thank you for this! ❤️

This PR does a lot more than just setup semconv.
In order to allow for better reviews, could you split it into multiple PRs?
From a first look, I'd do at least four:

  • Setup the body wrapper and use it.
  • Change the default span name behavior.
  • Introduce the semconv package, and use it with tracing only.
  • Introduce metrics support.

Striving for atomic PRs (each PR changes a single thing) makes for much speeder reviews.

@martinyonatann
Copy link
Contributor Author

martinyonatann commented Jan 20, 2025

thankyou for the feedback @dmathieu ! ❤️
You're absolutely right, this PR ended up covering more ground than initially intended. i appreciate your suggestion to split it into atomic PRs for easier review. i'll get started on breaking it down into the following :

This approach should make the review process smother and more efficient. i'll prioritize reorganizing these changes and will submit the individual PRs soon. Let me know if there are any additional points you'd like me to address.

@dmathieu dmathieu marked this pull request as draft January 20, 2025 09:27
@martinyonatann martinyonatann force-pushed the feat/migrate-to-semconv-for-otelmux branch from 50b35e9 to 379f764 Compare January 27, 2025 06:34
@martinyonatann martinyonatann changed the title migrate to semconv for otelmux #6638 migrate to semconv for otelmux and implementation the package in metrics Jan 27, 2025
@martinyonatann martinyonatann force-pushed the feat/migrate-to-semconv-for-otelmux branch from 379f764 to 225115d Compare January 30, 2025 14:40
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.9%. Comparing base (6676f17) to head (c2fe973).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6648     +/-   ##
=======================================
+ Coverage   73.8%   73.9%   +0.1%     
=======================================
  Files        195     195             
  Lines      16627   16671     +44     
=======================================
+ Hits       12285   12335     +50     
+ Misses      3982    3978      -4     
+ Partials     360     358      -2     
Files with missing lines Coverage Δ
...mentation/github.com/gorilla/mux/otelmux/config.go 100.0% <100.0%> (ø)
...trumentation/github.com/gorilla/mux/otelmux/mux.go 95.5% <100.0%> (+1.5%) ⬆️

... and 1 file with indirect coverage changes

@martinyonatann martinyonatann changed the title migrate to semconv for otelmux and implementation the package in metrics feat: support metrics for otelmux Jan 30, 2025
@martinyonatann martinyonatann marked this pull request as ready for review January 31, 2025 18:02
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Damien Mathieu <[email protected]>
@dmathieu dmathieu merged commit 8129e89 into open-telemetry:main Feb 5, 2025
25 checks passed
@MrAlias MrAlias added this to the v1.35.0 milestone Feb 6, 2025
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