Do not report on path label mapper not found

[mhodovaniuk]

Current behavior reports metric with path tag that may have big cardinality. This is a huge problem for monitoring. Moreover, whether the metric will be reported or not depends on the order in which we combine Http.

Closes #2294

[vigoo]

I think we should still count it as a request and tag with 404, just not adding the path tags.

[mhodovaniuk]

The problem is that if the handler sits in the second app of app1 ++ app2, app1 metrics will still report 404. Even that request will be handled and app2 metrics will report it as 200. In the end what we will have: one with 404 and one with 200 for single request.

[vigoo]

Yes the idea is that you apply the metrics middleware to the "final" app.
To support your use case it could be made an boolean option. I think we definitely need to support counting the not-found cases.

[mhodovaniuk]

So currently this is a vulnerability: if a service combines apps with metrics (which I believe most medium+ size services do) then it is possible to attack and cause OOM, or performance degradation, or even affect metrics service and as a result monitoring of other services. That means recording the path for unhandled requests is not an option at all for server metrics.

404 metric will make sense, only if it is applied to the final app. Otherwise, it will produce fake values. But It feels that it is hard to find a use case for this metric. I think such requests should be monitored by the client and not by the server, since the server can nothing to do to fix this. Also, without path information, this metric has limited usage. That is why I think it is better to remove it for server metrics.

If we want to keep the 404 metrics and apply metrics only to the final app. But there are two problems here:

1. Ideally would be to have compile time error when combining two apps metric. I think a note in the doc is not enough here.
2. Such a design will force developers to keep handlers and pathLabelMapper in different places which is also not very good. It will be easy to update handlers and forget about pathLabelMapper.

[vigoo]

That’s why I said make it an option. If you apply it to subsets or your routes you can turn it off, if you apply it once to the final app you can turn it on, if it has no value for you you can turn it off.

Seeing a change in the number of 404s may be useful in some cases so I definitely don’t want to remove this counter completely.

I fully agree about not adding the path as a label for it.

What’s your use case of applying the metrics middleware on parts of your route only?

[mhodovaniuk]

I would say what is the use case of declaring metrics middleware and as a result pathLabelMapper far away from routes. It is very error-prone and one may forget to update pathLabelMapper on route update. Additionally, maintaining this overarching pathLabelMapper could become challenging.

Furthermore, it's confusing that routes can be combined, and routes with regular middleware can also be combined, but routes with metrics middleware cannot be combined.

[jdegoes]

In my rewrite of Routes, I came to a similar conclusion as @mhodovaniuk: in the new rewrite, tracking of NotFound is not natural, because NotFound is not a handler, so therefore, middleware (which only applies to existing handlers) cannot track NotFound. In order to work around this, I had to add a fake NotFound handler, which then gets instrumented with metrics middleware (but only when it is added), which all felt a bit constrained.

I feel like if you want to treat NotFound as a route, you need to specifically add a route for it. And then when you do, it is very obviously not compositional, since an app only wants one NotFound handler, not multiple.

[jdegoes]

I would not merge this because of the rewrite, too much has changed.

[jdegoes]

I think this is superceded by #2320, so will close this one.