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

Takes into account the SCRIPT_NAME in path #204

Closed
wants to merge 1 commit into from

Conversation

woto
Copy link
Contributor

@woto woto commented Oct 2, 2020

Hi! Not sure that this is correct, but:

  1. We have lost the path using mountable engines (twirp, grape)
  2. It looks like the RFC says that this is correct: SCRIPT_NAME, PATH_NFO
  3. https://github.com/rack/rack/blob/master/lib/rack/handler/webrick.rb#L92

Sorry without tests for now. Want to hear your opinion.

Signed-off-by: Ruslan Kornev <oganer@gmail.com>
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4e63de0 on severgroup-tt:master into 872a8eb on prometheus:master.

@dmagliola
Copy link
Collaborator

Hi @woto,

Thank you for your PR.
This is the same issue from PR #197

Our concern with this is that it feels like it's a backwards incompatible change, and our recommendation at the time is to use this built-in collector as an "example" that you are encouraged to use as a base to make your own if it isn't fit for purpose.

I'd like to close the PR and move the conversation to #197 if that's ok, so we can have it in one place.
Thanks!

@woto
Copy link
Contributor Author

woto commented Oct 5, 2020

Ok, fine thank you

@dmagliola
Copy link
Collaborator

Ok, we've finally had time to look into this enough to convince ourselves this is definitely the right thing to do.
I wanted to add tests to this before merging, but I wasn't able to in this same branch, so I made a new PR #206
I've added you as a co-author on the commit, to give credit for the code and idea, hope that's ok!

I'm going to merge #206 son, and cut a new major version soon that fixes this.

Thank you for bringing this to our attention and for the PR!

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.

3 participants