-
Notifications
You must be signed in to change notification settings - Fork 502
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
Fix dropped metrics instruments #407
Conversation
Codecov Report
@@ Coverage Diff @@
## master #407 +/- ##
==========================================
+ Coverage 49.39% 49.40% +0.01%
==========================================
Files 66 66
Lines 5416 5416
==========================================
+ Hits 2675 2676 +1
+ Misses 2741 2740 -1
Continue to review full report at Codecov.
|
@@ -6,6 +6,33 @@ use opentelemetry::{ | |||
use opentelemetry_prometheus::PrometheusExporter; | |||
use prometheus::{Encoder, TextEncoder}; | |||
|
|||
#[test] | |||
fn free_unused_instruments() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm for some reason when I run this test against master
it works. Not sure if I missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior seems to have changed between dashmap version 4.0.0-rc6
and dashmap version 4.0.0
which was released this week. If you cargo update you should see the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha make sense. LGTM
@@ -6,6 +6,33 @@ use opentelemetry::{ | |||
use opentelemetry_prometheus::PrometheusExporter; | |||
use prometheus::{Encoder, TextEncoder}; | |||
|
|||
#[test] | |||
fn free_unused_instruments() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha make sense. LGTM
Currently metrics can deadlock when dropping the last reference to an instrument. This patch addresses this by switching to
https://docs.rs/dashmap/4.0.1/dashmap/struct.DashMap.html#method.retain
.Related #397