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

Use framework-specific route info and handle consecutive path segments containing IDs in Collector #245

Merged
merged 3 commits into from
Feb 5, 2022
Merged

Use framework-specific route info and handle consecutive path segments containing IDs in Collector #245

merged 3 commits into from
Feb 5, 2022

Conversation

Sinjo
Copy link
Member

@Sinjo Sinjo commented Feb 1, 2022 •
edited
Loading

Fixes #233

@Sinjo Sinjo force-pushed the sinjo-better-path-labels branch from 854c41e to 2f512f9 Compare February 1, 2022 02:27
Sinjo added 2 commits February 1, 2022 02:28
@Sinjo
Use framework-specific route info in collector when available
f1bb82a 
While `PATH_INFO` is framework agnostic, and works for any Rack app,
some Ruby web frameworks pass a more useful piece of information into
the request env - the route that the request matched.

This means that rather than using our generic `:id` and `:uuid`
replacements in the `path` label for any path segments that look like
dynamic IDs, we can put the actual route that matched in there, with
correctly named parameters. For example, if a Sinatra app defined a
route like:

get "/foo/:bar" do
  ...
end

instead of containing `/foo/:id`, the `path` label would contain
`/foo/:bar`.

Sadly, Rails is a notable exception, and (as far as I can tell at the
time of writing) doesn't provide this info in the request env.

Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
@Sinjo
Handle consecutive path segments containing IDs in collector
1b159e5 
Previously, we would fail to strip IDs from paths if they appeared in
consecutive path segments. This is because our regex that looked for IDs
and UUIDs would consume the `/` character that followed them, causing
the next one not to match.

This commit uses a lookahead to match against the `/` without consuming
it.

Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
@Sinjo Sinjo force-pushed the sinjo-better-path-labels branch from 2f512f9 to 1b159e5 Compare February 1, 2022 02:28
@Sinjo Sinjo requested a review from dmagliola February 1, 2022 02:28
@Sinjo Sinjo added this to the v3.0.0 milestone Feb 1, 2022
@Sinjo
Update UPGRADING.md
9d8ba56 
Add note about improvements to path label generation in
`Prometheus::Middleware::Collector`.

Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
@Sinjo
Copy link
Member Author

Sinjo commented Feb 1, 2022 •
edited
Loading

@dmagliola I've added the changes to UPGRADING.md to this PR as well, to save going through another one.

I'm going to add the CHANGELOG.md entry to #244 too.

@Sinjo Sinjo changed the title Use framework-specific route info and handle consecutive path segments containing IDs in collector Use framework-specific route info and handle consecutive path segments containing IDs in Collector Feb 1, 2022
@Sinjo Sinjo mentioned this pull request Feb 1, 2022
Closed
@Sinjo Sinjo merged commit 57feffb into master Feb 5, 2022
@Sinjo Sinjo deleted the sinjo-better-path-labels branch February 5, 2022 17:02
@Sinjo Sinjo mentioned this pull request Mar 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmagliola dmagliola dmagliola approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

better path handling
2 participants
@Sinjo @dmagliola