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

refactor: histogram #293

Closed
wants to merge 3 commits into from
Closed

refactor: histogram #293

wants to merge 3 commits into from

Conversation

chux0519
Copy link

@chux0519 chux0519 commented Nov 9, 2019

Open the PR.
Still have problem to be solved.
See the FIXME part in code

I just implmented generic version of some types in `histogram.rs`.The problem of loss of precision needs to be solved, perhaps make accurate to milliseconds when calling `observe` function.
@chux0519
Copy link
Author

chux0519 commented Nov 9, 2019

I just implemented generic version of some types in histogram.rs.
It will break one test case, because of the f64 to i64 casting.
I'm not sure how to fix this. is it OK to change the duration_to_seconds function to like duration_to_millis. Accurate to milliseconds and it should pass all tests.

@breeswish

@chux0519
Copy link
Author

chux0519 commented Nov 9, 2019

src/histogram.rs Outdated
@@ -397,21 +408,29 @@ impl HistogramTimer {
let v = duration_to_seconds(self.start.elapsed());
Copy link
Author

Choose a reason for hiding this comment

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

Accurate to milliseconds should fix this, is it OK to do so? @breeswish

src/histogram.rs Outdated
@@ -397,21 +408,29 @@ impl HistogramTimer {
let v = duration_to_seconds(self.start.elapsed());
self.observed = true;
if record {
self.histogram.observe(v);
// FIXME: 精度丢失
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use Chinese comments

Copy link
Author

Choose a reason for hiding this comment

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

sure, fixed in last commit

- make `DEFAULT_BUCKETS` accurate to milliseconds
- and adapted to relevant test cases
src/histogram.rs Outdated
core: Arc<HistogramCore<S, C>>,
}

/// /// A [`Metric`](::core::Metric) counts individual observations from an event or sample stream in
Copy link
Author

Choose a reason for hiding this comment

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

duplicate /// mark

src/histogram.rs Outdated
@@ -397,21 +408,29 @@ impl HistogramTimer {
let v = duration_to_seconds(self.start.elapsed());
self.observed = true;
if record {
self.histogram.observe(v);
// FIXME: 精度丢失
Copy link
Author

Choose a reason for hiding this comment

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

sure, fixed in last commit

also fix duplicated `///` mark
@breezewish
Copy link
Member

@chux0519 Thanks for the PR! Looks like your PR makes a breaking change that previously Histogram reports durations in seconds but now it reports in milliseconds. You may consider adding a new interface that records millisecond precision.

@chux0519
Copy link
Author

chux0519 commented Dec 4, 2019

@chux0519 Thanks for the PR! Looks like your PR makes a breaking change that previously Histogram reports durations in seconds but now it reports in milliseconds. You may consider adding a new interface that records millisecond precision.

you are right, I will definitely fix this

@mxinden
Copy link
Contributor

mxinden commented Sep 10, 2020

@chux0519 are you planning to continue the work on this pull request? If not, I would close here.

@chux0519
Copy link
Author

@chux0519 are you planning to continue the work on this pull request? If not, I would close here.

Sorry, I may not be able to continue this PR, it can be closed

@lucab lucab closed this Sep 11, 2020
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.

5 participants