Skip to content
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

feat(metrics/family): 🍫 add Family::get_or_create_owned() #244

Merged
merged 5 commits into from
Feb 9, 2025

Conversation

cratelyn
Copy link
Contributor

fixes #231.

this introduces a new method to Family<S, M, C> to help alleviate the risk of deadlocks when accessing multiple series within a given metrics family.

this returns a plain M, rather than the MappedRwLockReadGuard<M> RAII guard returned by get_or_create().

a test case is introduced in this commit to demonstrate that structures accessing multiple series within a single expression will not accidentally create a deadlock.

@cratelyn cratelyn force-pushed the family-get-or-create-clone branch 2 times, most recently from 13dbb03 to fb14089 Compare November 18, 2024 23:12
@cratelyn cratelyn force-pushed the family-get-or-create-clone branch from fb14089 to 708c1ec Compare January 21, 2025 03:38
@cratelyn
Copy link
Contributor Author

i've rebased this commit to address merge conflicts, and added information to the changelog. let me know if there is anything else that needs to be done. i am keen on a release that addresses the deadlocks reported in #231.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this.

src/metrics/family.rs Outdated Show resolved Hide resolved
src/metrics/family.rs Outdated Show resolved Hide resolved
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more comments. Otherwise looks good to me.

Agreed, that this is not the ideal solution, but in my eyes a good compromise, given that it is not a breaking change. Long term, we can redesign this interface in a breaking release.

src/metrics/family.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cratelyn added a commit to cratelyn/prometheus-client that referenced this pull request Feb 9, 2025
prometheus#244 (comment)

Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
fixes prometheus#231.

this introduces a new method to `Family<S, M, C>` to help alleviate the
risk of deadlocks when accessing multiple series within a given metrics
family.

this returns a plain `M`, rather than the `MappedRwLockReadGuard<M>`
RAII guard returned by `get_or_create()`.

a test case is introduced in this commit to demonstrate that structures
accessing multiple series within a single expression will not
accidentally create a deadlock.

Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
this linked to the wrong method. this is now fixed.

Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
prometheus#244 (comment)

Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
@cratelyn cratelyn force-pushed the family-get-or-create-clone branch from 9e34bd3 to 4f11006 Compare February 9, 2025 18:54
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Signed-off-by: Max Inden <mail@max-inden.de>
@mxinden mxinden changed the title feat(metrics/family): 🍫 add Family::get_or_create_clone() feat(metrics/family): 🍫 add Family::get_or_create_owned() Feb 9, 2025
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Max Inden <mail@max-inden.de>
@mxinden mxinden merged commit 377ca2d into prometheus:master Feb 9, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deadlock when using Family::get_or_create
2 participants