Remove framework-specific route detection from collector middleware #251
Conversation
This file contains 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
|
Sure thing. Maybe a slight rearrange of this doc to emphasise that you can customise |
|
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>
|
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