Remove framework-specific route detection from collector middleware #251
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sinjo force-pushed the sinjo-remove-framework-specific-routing branch from 20a21e7 to a26ae7d Compare 
Sadly, with the benefit of hindsight, this wasn't a good idea. There are two reasons we're dropping this: - It doesn't play nicely with libraries like `Rack::Builder`, which dispatches requests to different Rack apps based on a path prefix in a way that isn't visible to middleware. For example, when using `Rack::Builder`, `sinatra.route` only contains the parts of the path after the prefix that `Rack::Builder` used to dispatch to the specific app, and doesn't leave any information in the request environment to indicate which prefix it dispatched to. - It turns out framework-specific route info isn't always formatted in a way that looks good in a Prometheus label. For example, when using regex-based route-matching in Sinatra, you can end up with labels that look like `\\/vapes\\/([0-9]+)\\/?`. For a really detailed dive into those two issues, see this GitHub comment: #249 (comment) Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
Sinjo force-pushed the sinjo-remove-framework-specific-routing branch from a26ae7d to e0dd0f9 Compare 
dmagliola approved these changes Mar 11, 2022
Collaborator
dmagliola left a comment
dmagliola left a comment There was a problem hiding this comment.
Should we document this new idea that if you want specific path labeing, the way to go is subclassing the Collector and adding your class as the middleware instead?
Member Author
| Sure thing. Maybe a slight rearrange of this doc to emphasise that you can customise |
Collaborator
| Yeah, that sounds perfect. |
Sinjo force-pushed the sinjo-remove-framework-specific-routing branch from 1533fbd to 1dfc1ad Compare Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
Member Author
| All done! |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Sadly, with the benefit of hindsight, this wasn't a good idea.
There are two reasons we're dropping this:
It doesn't play nicely with libraries like
Rack::Builder, whichdispatches requests to different Rack apps based on a path prefix in
a way that isn't visible to middleware.
For example, when using
Rack::Builder,sinatra.routeonlycontains the parts of the path after the prefix that
Rack::Builderused to dispatch to the specific app, and doesn't leave any
information in the request environment to indicate which prefix it
dispatched to.
It turns out framework-specific route info isn't always formatted in
a way that looks good in a Prometheus label.
For example, when using regex-based route-matching in Sinatra, you
can end up with labels that look like
\\/vapes\\/([0-9]+)\\/?.For a really detailed dive into those two issues, see this GitHub
comment:
#249 (comment)
Fixes #249