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

Fix #1457 by adding context.MatchedRoute() method #1502

Closed
wants to merge 1 commit into from

Conversation

lammel
Copy link
Contributor

@lammel lammel commented Feb 8, 2020

This PR fixes issue #1457 by adding a MatchedRoute method to context that will only return the path of a registered route.
This will allow for easier identification of matched routes and metric providers to return a limited set of known routes.

The method is based on code that we used as a helper method to filter known paths for providing metrics of requested URIs.

Tests have been added.

@codecov
Copy link

codecov bot commented Feb 8, 2020

Codecov Report

Merging #1502 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1502      +/-   ##
==========================================
+ Coverage   84.38%   84.46%   +0.08%     
==========================================
  Files          27       27              
  Lines        2094     2099       +5     
==========================================
+ Hits         1767     1773       +6     
+ Misses        213      212       -1     
  Partials      114      114
Impacted Files Coverage Δ
context.go 91.37% <100%> (+0.2%) ⬆️
echo.go 85.75% <0%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75620e6...3103e4e. Read the comment docs.

@lammel lammel mentioned this pull request Feb 9, 2020
3 tasks
@djgilcrease
Copy link

djgilcrease commented Feb 9, 2020

I would make a MetricsContext interface with this in it, note the interface as potentially unstable while all the methods are determined for it and still have the default context implementation for the method. That way the default context satisfies both the Context and MetricsContext and people who want to get the metrics can cast the context. This also makes it so those who are implementing their own context do not break when this releases.

In v5 all the context interfaces could be merged if that is determined to be the best course.

@lammel
Copy link
Contributor Author

lammel commented Feb 24, 2020

@vishr Can you add the v5 label on this issue please

@vishr vishr added the v5 label Feb 24, 2020
@vishr
Copy link
Member

vishr commented Feb 24, 2020

@lammel done!

@stale
Copy link

stale bot commented Apr 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 25, 2020
@stale stale bot closed this May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants