Include SCRIPT_NAME when determining path

[ianks]

When determining the path for a request, Rack::Request prefixes the SCRIPT_NAME, as seen here. This makes sure the relative path is included. At work, we use Iodine and Turbolinks, and without this patch most of our requests paths in Prometheus are "". This patch fixes that.

[coveralls]

Coverage remained the same at 100.0% when pulling f50ee67 on ianks:include-script-name-in-path into 872a8eb on prometheus:master.

[dmagliola]

Hi Ian,

We provide this middleware as a sort of "example" / starting point on how to collect Rack metrics. We know it doesn't cater for every use case, and we encourage people to use it as a base to make their own version, if this one doesn't suit. Documented here

We, for example, don't use this one, we have our own version that is quite similar, but does a few more checks on the path before exposing metrics.

I'm not sure your change is generic enough that it will work for everyone, and won't break existing working cases. This may be a backwards incompatible change, for example, in which case i'm not too keen on adding this.

For this use case, i'd encourage you to copy this collector into your codebase, and make these modifications in your project, rather than upstream in the gem.

As for the second commit... (I consider this a separate change completely, worth discussing individually)

Are you just reducing duplication there? Or is this actually measurably faster than what the existing code does? (i'm not sure which meaning of "optimize" you're going for in the commit message)
If it's the former, I actually find the existing code to be much easier to read, so not sure about this change.

Maybe we should extract the call to strip_ids_from_path outside the Hash declaration and store it in a local var, so we're not calling that twice, if that's measurably faster. If you want to open a separate PR with that, i'd be happy to review that :-)

Thanks!
Daniel

[ianks]

We provide this middleware as a sort of "example" / starting point on how to collect Rack metrics. We know it doesn't cater for every use case, and we encourage people to use it as a base to make their own version, if this one doesn't suit. Documented here

I would imagine for a lot of people, the current implementation would not have the expected behavior. Also, pulling this into our codebase is not ideal for a couple of reasons:

1. It adds more non-application code we have to maintain
2. By default, backtraces for Rack middleware will show up for all request errors when the code is not in a Gem. This makes debugging actual issues much more tedious.

I'm not sure your change is generic enough that it will work for everyone, and won't break existing working cases. This may be a backwards incompatible change, for example, in which case i'm not too keen on adding this.

This is a good point. Would you be willing to merge a backwards compatible version that took an option to mimic Rack::Request#path?

Are you just reducing duplication there? Or is this actually measurably faster than what the existing code does? (i'm not sure which meaning of "optimize" you're going for in the commit message)

I was just removing the rendundant call to strip_ids_from_path, I can break this out if you want.

[dmagliola]

I would imagine for a lot of people, the current implementation would not have the expected behavior.

We haven't heard other cases of this particular behaviour being an issue, and we do encourage making your own collector if you have special needs.

It adds more non-application code we have to maintain

In my opinion, it's not a huge amount of code to bring in. We're doing it at our company and haven't had an issue with it.

By default, backtraces for Rack middleware will show up for all request errors when the code is not in a Gem.
This makes debugging actual issues much more tedious.

That is indeed annoying. One option might be to make a small gem with just the collector, maybe

Would you be willing to merge a backwards compatible version that took an option to mimic Rack::Request#path?

My concern is I don't see how we can have a backwards-compatible version, which simultaneously behaves the same as the current code, and also fixes your issue. I feel like i'm missing something here.

I was just removing the rendundant call to strip_ids_from_path, I can break this out if you want.

Have you measured what kind of actual performance impact this has? Unless it's a measurable difference, I think the existing code is much more readable.
If you can benchmark a simple app that goes through Rack, with these two versions of the code, and the one that doesn't duplicate that code is actually faster, then yes, please, open a PR with those benchmarks and we'll merge that in.

To clarify: I don't mean measuring just the two ways of generating those hashes, I mean measuring that as part of actual Rack requests, because even if one version is 100 times faster, if the simpler-to-read code is taking an insignificant amount of time in the context of a Rack request, then I still think we should keep the more readable code.

[dmagliola]

A contributor opened another PR (#204) to address this same issue.

Some supporting evidence that this may be the way to go (from their PR's description): https://github.com/rack/rack/blob/master/lib/rack/handler/webrick.rb#L92

[ianks]

Given now that there are multiple PRs attempting to address this issue, it makes me wonder how many have encountered the issue and haven't PR'd 😄

Would you consider including this change in the next major release?

[dmagliola]

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 close this and merge #206, and cut a new major version soon that fixes this.

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