Skip to content

Make it possible to conditionally skip any histogram observation #2313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

klippx
Copy link
Contributor

@klippx klippx commented Oct 10, 2024

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.

  • 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

  • Should allow user to decide if an observation should be made

Checklist:

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

Copy link

changeset-bot bot commented Oct 10, 2024

🦋 Changeset detected

Latest commit: a8bc62a

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 klippx force-pushed the prometheus-pre-observe-hook branch from f5c0c44 to a8bc62a Compare October 10, 2024 09:30
@klippx klippx changed the title Make it possible for a user to conditionally escape histogram/summary… Make it possible to conditionally skip any histogram observation Oct 10, 2024
@klippx
Copy link
Contributor Author

klippx commented Oct 22, 2024

@EmrysMyrddin May I ask for review?

@EmrysMyrddin
Copy link
Collaborator

EmrysMyrddin commented Oct 23, 2024

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
Copy link
Contributor Author

klippx commented Oct 25, 2024

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

@klippx
Copy link
Contributor Author

klippx commented Nov 5, 2024

Closing in favor of #2317

@klippx klippx closed this Nov 5, 2024
@klippx klippx deleted the prometheus-pre-observe-hook branch November 19, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[@envelop/plugin-prometheus] Need a way to filter which metrics are recorded
2 participants