Skip to content

Migrate to semconv for otelmux and implement the package in tracing#6652

Merged
dmathieu merged 10 commits into
open-telemetry:mainfrom
martinyonatann:feat/semconv-for-otelmux
Feb 3, 2025
Merged

Migrate to semconv for otelmux and implement the package in tracing#6652
dmathieu merged 10 commits into
open-telemetry:mainfrom
martinyonatann:feat/semconv-for-otelmux

Conversation

@martinyonatann
Copy link
Copy Markdown
Contributor

@martinyonatann martinyonatann commented Jan 20, 2025

This PR updates the otelmux package by introducing the semconv package and using it with tracing only. It also updates the template and refines the gen.go in otelhttp to reflect the changes made in the template.

This PR addresses feedback provided in #6648

@martinyonatann martinyonatann requested a review from a team as a code owner January 20, 2025 20:20
@github-actions github-actions Bot requested a review from akats7 January 20, 2025 20:20
@martinyonatann
Copy link
Copy Markdown
Contributor Author

I had to delete the semconv gen.go file because when running make precommit, the otelhttp library was re-imported, which broke the generated code.

@martinyonatann martinyonatann changed the title migrate to semconv for otelmux migrate to semconv for otelmux and update semconv template to use convert value Jan 21, 2025
@martinyonatann martinyonatann changed the title migrate to semconv for otelmux and update semconv template to use convert value migrate to semconv for otelmux and add parameter to semconv template Jan 21, 2025
@martinyonatann
Copy link
Copy Markdown
Contributor Author

I had to delete the semconv gen.go file because when running make precommit, the otelhttp library was re-imported, which broke the generated code.

Solved by adding a parameter in the template.

@dmathieu
Copy link
Copy Markdown
Member

dmathieu commented Jan 22, 2025

Thank you for the template fix. I've extracted it into another PR, so other instrumentations aren't blocked by this one. #6663

@martinyonatann martinyonatann changed the title migrate to semconv for otelmux and add parameter to semconv template Migrate to semconv for otelmux and implement the package in tracing Jan 22, 2025
@martinyonatann martinyonatann force-pushed the feat/semconv-for-otelmux branch 2 times, most recently from 15a23fb to 100f343 Compare January 26, 2025 17:08
@martinyonatann martinyonatann force-pushed the feat/semconv-for-otelmux branch from 100f343 to 281e23d Compare January 26, 2025 17:09
Comment thread instrumentation/github.com/gorilla/mux/otelmux/config.go Outdated
Comment thread internal/shared/semconv/env.go.tmpl Outdated
Comment thread instrumentation/github.com/gorilla/mux/otelmux/mux.go Outdated
@dmathieu
Copy link
Copy Markdown
Member

This will need a changelog entry.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 87.91774% with 94 lines in your changes missing coverage. Please review.

Project coverage is 73.8%. Comparing base (bf0e3dd) to head (94b748b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...m/gorilla/mux/otelmux/internal/semconv/httpconv.go 87.0% 36 Missing and 15 partials ⚠️
...om/gorilla/mux/otelmux/internal/semconv/v1.20.0.go 88.4% 13 Missing and 7 partials ⚠️
...ub.com/gorilla/mux/otelmux/internal/semconv/env.go 89.6% 11 Missing and 3 partials ⚠️
...b.com/gorilla/mux/otelmux/internal/semconv/util.go 89.4% 4 Missing and 2 partials ⚠️
...trumentation/github.com/gorilla/mux/otelmux/mux.go 84.2% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6652     +/-   ##
=======================================
+ Coverage   73.1%   73.8%   +0.7%     
=======================================
  Files        191     195      +4     
  Lines      15867   16627    +760     
=======================================
+ Hits       11607   12285    +678     
- Misses      3925    3982     +57     
- Partials     335     360     +25     
Files with missing lines Coverage Δ
...trumentation/github.com/gorilla/mux/otelmux/mux.go 94.0% <84.2%> (+9.0%) ⬆️
...b.com/gorilla/mux/otelmux/internal/semconv/util.go 89.4% <89.4%> (ø)
...ub.com/gorilla/mux/otelmux/internal/semconv/env.go 89.6% <89.6%> (ø)
...om/gorilla/mux/otelmux/internal/semconv/v1.20.0.go 88.4% <88.4%> (ø)
...m/gorilla/mux/otelmux/internal/semconv/httpconv.go 87.0% <87.0%> (ø)

Comment thread instrumentation/github.com/gorilla/mux/otelmux/mux.go Outdated
@martinyonatann martinyonatann force-pushed the feat/semconv-for-otelmux branch from 3ff7e4e to b1225a4 Compare January 28, 2025 09:20
@akats7
Copy link
Copy Markdown
Contributor

akats7 commented Jan 28, 2025

Will review after body wrapper is merged/included

@dmathieu
Copy link
Copy Markdown
Member

Can you fix the conflicts that were introduced with the merge of your other PR?

@martinyonatann
Copy link
Copy Markdown
Contributor Author

done. please help to review again @dmathieu @akats7

Comment thread CHANGELOG.md Outdated
Comment thread instrumentation/github.com/gorilla/mux/otelmux/mux.go Outdated
@dmathieu dmathieu merged commit b04473d into open-telemetry:main Feb 3, 2025
@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