search

jochen-schweizer / express-prom-bundle

express middleware with standard prometheus metrics in one bundle
MIT License
303 stars 67 forks source link

fix: regex: stop when match #48

Closed caarlos0 closed 3 years ago

caarlos0 commented 4 years ago

Hi, thanks for this lib!

One thing I found to not be very obvious is that the regexps are actually all applied, while I would expect it to stop on the first one that matches.

Stopping on the first one makes it easier to handle "precedence" and also avoids the negative lookaheads and etc to grab things on the middle of the path.

For instance, its hard to do things like this right now:

normalizePath: [
    ['^/v(\\d)/projects/.*/alias$', '/v$1/projects/#name/alias'],
    ['^/v(\\d)/projects/.*', '/v$1/projects/#name']
]

this will actually put all metrics in /v$1/projects/#name.

I understand this is a breaking change and I'm willing to change whatever you think its needed, and would also understand if you don't want to accept the PR at all.

Thanks!

caarlos0 commented 4 years ago

gently ping @jochen-schweizer

disjunction commented 4 years ago

@caarlos0 I'm afraid it would break another use case where you have rules like:

[
  ['/projects/[^/]+/', '/projects/#name/'],
  ['/ticket/[^/]+/', '/ticket/#name'],
 ...
]

btw m.b. such an approach would also help you. Note, that you don't have to match the entire string each time.

ATM I'm inclined to keep the existing approach

caarlos0 commented 4 years ago

Yeah, I'm doing something like that too, but having it stop on first match would make the REs simpler.

Maybe we can add that an option? WDYT?

disjunction commented 3 years ago

it is a potentially breaking change. So I keep the existing logic. For special cases like in OP please use a custom function which you can provide instead of the array.

(and sorry - for the misplaced comment I put here before)