Make it possible to conditionally skip any histogram observation

klippx commented 1 month ago

Description

Make it possible for a user to conditionally escape histogram/summary observations on a per request basis.

Fixes #2312

Type of change

Please delete options that are not relevant.

[x] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

[x] Should allow user to decide if an observation should be made

Checklist:

[x] I have followed the CONTRIBUTING doc and the style guidelines of this project
[] I have performed a self-review of my own code
[x] I have commented on my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[x] Any dependent changes have been merged and published in downstream modules

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: a8bc62a09c39079dc7820acb0255da75400cdeb5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | ------------------- | ----- | | @envelop/prometheus | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

klippx commented 3 weeks ago

@EmrysMyrddin May I ask for review?

EmrysMyrddin commented 3 weeks ago

Hi, yes ! Sorry for the delay, I went down a rabbit hole on this subject. I started to test your PR and came across a lot of weird behavior, where the config can be very confusing: some metric will ba exported only if some other metrics are also available.

For this reason, I've started a PR to rework the way metrics are enabled/disabled and more importantly how they will be enabled based on the execution phase. This allows us to optimize the plugin, by not adding hooks for phases that are not needed by any enabled metric.

This PR will also add a way to skip metric observations based on the execution context.

The work is not finalized yet (you can see the draft PR linked in your issue), because we are still discussing the shape of the configuration API (how much do we want to break the retro-compatibility with previous versions).

klippx commented 2 weeks ago

Thanks @EmrysMyrddin you are a legend... ❤️ I agree 100% with your take. Looking forward to it!

klippx commented 1 week ago

Closing in favor of #2317

n1ru4l / envelop