search

nlopes / actix-web-prom

Actix-web middleware to expose Prometheus metrics
MIT License
89 stars 56 forks source link

Added http_version label to metrics. #74

Closed mstyura closed 10 months ago

mstyura commented 11 months ago

When you provide http service to other services it's important to understand which http version is actually used by calling services. I've introduced additional label to metrics with the http version of served request. So basically how the whole "first" line of http request body like GET /test HTTP/2 and response status is reflected in metrics. Hope this is not that much breaking change to be rejected. Thanks a lot in advance for a review.

mstyura commented 11 months ago

Hello @nlopes! Could you please take a look and decide is it acceptable to have such change to metrics? Thanks a lot in advance!

nlopes commented 11 months ago

Thanks for submitting, I'll try to take a look at this this week.

nlopes commented 11 months ago

@mstyura I briefly looked at this and I'm not sure this is an addition I'm comfortable making.

~I'd say using const_labels will give you the same and it's already customisable by the consumer of this library. Which means for people that don't mind increased cardinality they can add these, for people that do mind, the library is conservative by default.~

I know it's not a sure way due to potentially answering on multiple http versions - I wonder if there's a way that means the library stays conservative. Maybe through a flag?

mstyura commented 11 months ago

I know it's not a sure way due to potentially answering on multiple http versions - I wonder if there's a way that means the library stays conservative. Maybe through a flag?

Would you mind if I try to implement such flags via crate's features flag or you prefer runtime configuration flag? Regarding multiple versions, I expect there will be increased demand on tracking http version, because actix-web has recently added auto_h2c feature, and having observability on what http is actually used might become important.

nlopes commented 11 months ago

I know it's not a sure way due to potentially answering on multiple http versions - I wonder if there's a way that means the library stays conservative. Maybe through a flag?

Would you mind if I try to implement such flags via crate's features flag or you prefer runtime configuration flag? Regarding multiple versions, I expect there will be increased demand on tracking http version, because actix-web has recently added auto_h2c feature, and having observability on what http is actually used might become important.

One way or another that would be nice. I think runtime flag (maybe through the Builder) would be better in this case as it doesn't break the library contract.

mstyura commented 11 months ago

@nlopes thank you very much you've taken a look at original PR, I've updated pull request with proposed solution based on runtime builder flag. Could you please give another chance to PR and have a look? Thanks a lot!