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

Fix dropped metrics instruments #407

Merged
merged 1 commit into from
Dec 31, 2020

Conversation

jtescher
Copy link
Member

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

@jtescher jtescher requested a review from a team December 31, 2020 04:45
@jtescher jtescher changed the title Fix metrics dropped instruments Fix dropped metrics instruments Dec 31, 2020
@codecov
Copy link

codecov bot commented Dec 31, 2020

Codecov Report

Merging #407 (707bf5d) into master (2e32e41) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
opentelemetry/src/sdk/metrics/mod.rs 0.00% <0.00%> (ø)
...ntelemetry/src/sdk/metrics/aggregators/ddsketch.rs 76.95% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e32e41...707bf5d. Read the comment docs.

@@ -6,6 +6,33 @@ use opentelemetry::{
use opentelemetry_prometheus::PrometheusExporter;
use prometheus::{Encoder, TextEncoder};

#[test]
fn free_unused_instruments() {
Copy link
Contributor

@TommyCpp TommyCpp Dec 31, 2020

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.

Copy link
Member Author

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.

Copy link
Contributor

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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha make sense. LGTM

@jtescher jtescher merged commit e8c1c02 into master Dec 31, 2020
@jtescher jtescher deleted the fix-metrics-unreferenced-instruments branch December 31, 2020 18:51
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.

2 participants