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.

Sign up for GitHub

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

Jump to bottom

WithRouteTag adds HTTP route attribute to metrics #615

Merged
merged 12 commits into from Jul 31, 2023
Merged

WithRouteTag adds HTTP route attribute to metrics #615

merged 12 commits into from Jul 31, 2023

Conversation

charleskorn
Copy link
Contributor

Resolves #611.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 14, 2021 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Mar 14, 2021 •
edited

Codecov Report

Merging #615 (d6ae09e) into main (bf2ae27) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #615   +/-   ##
=====================================
  Coverage   79.3%   79.4%           
=====================================
  Files        165     165           
  Lines      10318   10324    +6     
=====================================
+ Hits        8183    8198   +15     
+ Misses      2000    1992    -8     
+ Partials     135     134    -1
Files Changed Coverage Δ
instrumentation/net/http/otelhttp/handler.go 86.9% <100.0%> (+3.8%) ⬆️

... and 1 file with indirect coverage changes

@charleskorn
Copy link
Contributor Author

Would it be possible to get a review on this? I'd love to see this functionality implemented.

@RangelReale
Copy link
Contributor

I'm also needing something like this.

MrAlias
MrAlias reviewed Nov 3, 2022
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, these changes look good. Please sync with main.

instrumentation/net/http/otelhttp/handler_test.go Outdated Show resolved Hide resolved
@charleskorn charleskorn requested review from MrAlias and removed request for lizthegrey November 19, 2022 15:43
@changliu-wk
Copy link

Any concerns about the change? Really want to have this feature. @charleskorn

@pellared
Copy link
Member

@charleskorn Can you update the PR? Sorry for the delay.

@charleskorn charleskorn requested a review from a team as a code owner May 28, 2023 00:24
@charleskorn
Copy link
Contributor Author

@charleskorn Can you update the PR? Sorry for the delay.

No worries, done!

pellared
pellared reviewed May 29, 2023
View reviewed changes
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice. I just left a few minor comments that I think are worth addressing.

instrumentation/net/http/otelhttp/test/handler_test.go Outdated Show resolved Hide resolved
instrumentation/net/http/otelhttp/test/handler_test.go Outdated Show resolved Hide resolved
instrumentation/net/http/otelhttp/test/handler_test.go Outdated Show resolved Hide resolved
instrumentation/net/http/otelhttp/test/handler_test.go Show resolved Hide resolved
@pellared pellared changed the title Add route tag to metrics as well as traces when using otelhttp.WithRouteTag. WithRouteTag adds HTTP route attribute to metrics Jul 31, 2023
@pellared
Copy link
Member

@charleskorn Thanks for your contribution 🎉

@pellared
Copy link
Member

I am planning to merge this PR. There are only some problems with EasyCLA integration which is blocking this PR. This problem affects other PRs as well.

@pellared pellared closed this Jul 31, 2023
@pellared pellared reopened this Jul 31, 2023
@pellared pellared merged commit 525d6c0 into open-telemetry:main Jul 31, 2023
41 checks passed
@MrAlias MrAlias added this to the v0.43.0 milestone Aug 28, 2023
@MrAlias MrAlias mentioned this pull request Aug 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmathieu dmathieu dmathieu approved these changes

@dashpole dashpole dashpole approved these changes

@pellared pellared pellared approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9 Aneurysm9 is a code owner

@evantorrie evantorrie Awaiting requested review from evantorrie

@jmacd jmacd Awaiting requested review from jmacd

@XSAM XSAM Awaiting requested review from XSAM

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

@MrAlias MrAlias Awaiting requested review from MrAlias

Assignees
No one assigned
Labels
instrumentation: otelhttp
Projects
None yet
Milestone
v0.43.0
Development

Successfully merging this pull request may close these issues.

otelhttp.WithRouteTag does not set route tag for metrics
7 participants
@charleskorn @RangelReale @changliu-wk @pellared @dmathieu @dashpole @MrAlias