Skip to content

Conversation

@dmagliola
Copy link
Collaborator

When determining the path for a request, Rack::Request prefixes the
SCRIPT_NAME, as seen here.

This is a problem with our current code when using mountable engines,
where the engine part of the path gets lost.

This patch fixes that to include SCRIPT_NAME as part of the path.

NOTE: This is not backwards compatible. Labels will change in existing
metrics. We will cut a new major version once we ship this.

When determining the path for a request, `Rack::Request` prefixes the
`SCRIPT_NAME`, [as seen here][1].

This is a problem with our current code when using mountable engines,
where the engine part of the path gets lost.

This patch fixes that to include `SCRIPT_NAME` as part of the path.

NOTE: This is not backwards compatible. Labels will change in existing
metrics. We will cut a new major version once we ship this.

[1]: https://github.com/rack/rack/blob/294fd239a71aab805877790f0a92ee3c72e67d79/lib/rack/request.rb#L512

Co-authored-by: Ian Ker-Seymer <[email protected]>
Co-authored-by: Ruslan Kornev <[email protected]>
Signed-off-by: Daniel Magliola <[email protected]>
@dmagliola dmagliola force-pushed the add_script_name_in_collector_path branch from d14e4dd to 0761f59 Compare October 14, 2020 10:12
@coveralls
Copy link

coveralls commented Oct 14, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 0761f59 on add_script_name_in_collector_path into 726536c on master.

@dmagliola
Copy link
Collaborator Author

@ianks @woto Thank you for your PRs! I couldn't push tests into your branches, so made this new PR and tagged you as co-authors in the commit. I hope that's ok with you.
Hoping to merge this soon

@dmagliola dmagliola merged commit 4cdd0ab into master Oct 14, 2020
@dmagliola dmagliola deleted the add_script_name_in_collector_path branch October 26, 2020 18:27
@ianks
Copy link
Contributor

ianks commented Jan 12, 2021

Awesome stuff. Excited for the new major!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants