search

jochen-schweizer / express-prom-bundle

express middleware with standard prometheus metrics in one bundle
MIT License
310 stars 68 forks source link

feat: respect pruneAgedBuckets #119

Closed timm-stelzer-e-farm closed 10 months ago

disjunction commented 1 year ago

Hi Timm. This looks like a nice feature. I will test and integrate it in the coming days.

Yet I'm a bit concerned about bumping the required prom-client version. It will make a lot of existing projects incompatible. I wonder if having the support for the pruneAgedBuckets has any problems with the older client? I would expect not, in other words if people have the new prom-client then it will work, otherwise just ignored. What do you think?

timm-stelzer-e-farm commented 1 year ago

Yet I'm a bit concerned about bumping the required prom-client version.

Yes, I agree, and as a user I would probably expect a major version bump for this, unless we know the versions are backwards-compatible.

has any problems with the older client?

Couldn't say, @rilpires implemented it in prom-client, AFAIK.

From briefly looking at the code, I would expect the option to be simply ignored, if using an older prom-client, given its an addition. So maybe we don't need the prom-client version bump here.

rilpires commented 1 year ago

Just my 2 cents,

From briefly looking at the code, I would expect the option to be simply ignored

Yes, it will be simply ignored. It is just a Object.assign on top of a default config object

So maybe we don't need the prom-client version bump here.

Well, the pruneAgedBuckets feature just won't be available then.

timm-stelzer-e-farm commented 1 year ago

Removed the version bump and documented the requirement in the README.

rilpires commented 1 year ago

This is the count of http_request_duration_seconds_count in some of our web servers after this feature was enabled :satisfied: image

disjunction commented 10 months ago

Merged manually. Thanks for contributing. This was released in v7.0.0