Skip to content

Conversation

@ljbade
Copy link

@ljbade ljbade commented Mar 25, 2022

This PR is set to merge on the rebased master, but I will update that once #7 is merged.

I found the use of Seconds() confusing for the metric expiry time, so I renamed it TTL().

I also updated the expiry code use use std::chrono as that is already used in the Summary class, and makes the TTL units clearer.

Finally I added a bunch of test cases to check that the expiry stuff is working correctly.

@ljbade ljbade requested a review from jbangelo March 25, 2022 03:55
@coveralls
Copy link

coveralls commented Mar 25, 2022

Pull Request Test Coverage Report for Build 2063488925

  • 17 of 18 (94.44%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 96.306%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/src/gauge.cc 4 5 80.0%
Totals Coverage Status
Change from base Build 2062151750: 0.2%
Covered Lines: 756
Relevant Lines: 785

💛 - Coveralls

@ljbade ljbade force-pushed the ljbade/ttl branch 4 times, most recently from 34e469b to 7cebf7d Compare March 25, 2022 12:11
@ljbade
Copy link
Author

ljbade commented Mar 28, 2022

I will be needing the chrono stuff as well for PR I am currently working on where I need to feed in external time to the summary metric type.

@ljbade ljbade force-pushed the ljbade/rebase-upstream branch from 6837162 to dd539fd Compare March 28, 2022 22:42
@ljbade ljbade force-pushed the ljbade/ttl branch 2 times, most recently from 07a5a53 to 9ba4fa8 Compare March 28, 2022 23:07
@ljbade ljbade requested a review from silverjam March 28, 2022 23:21
@ljbade ljbade force-pushed the ljbade/ttl branch 2 times, most recently from d13138c to 100efce Compare March 29, 2022 12:22
Copy link

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

This look even better for an upstream PR submission, in general we should make reasonable effort to get things upstream so that our forks are as minimal as possible

@silverjam silverjam requested a review from notoriaga March 29, 2022 22:20
@ljbade ljbade force-pushed the ljbade/rebase-upstream branch 2 times, most recently from b389800 to 3fc5e1a Compare March 30, 2022 01:01
@ljbade
Copy link
Author

ljbade commented Mar 30, 2022

@silverjam yep I think this should be good for upstream. I think replacing steady clock with system clock might be controversial but that was the only way I could then pass in time stamps from recorded data in for replay. I did try to think if there was a way to do make it flexible with templates but ended up a bit of a mess.

@silverjam
Copy link

@silverjam yep I think this should be good for upstream. I think replacing steady clock with system clock might be controversial but that was the only way I could then pass in time stamps from recorded data in for replay. I did try to think if there was a way to do make it flexible with templates but ended up a bit of a mess.

Hrmm, OK, maybe we could upstream everything except that change then?

@ljbade
Copy link
Author

ljbade commented Mar 30, 2022

Yeah I think that would make sense. I feel like us replaying data is probably a bit of a corner case for this library...

Base automatically changed from ljbade/rebase-upstream to master March 30, 2022 02:01
@ljbade ljbade force-pushed the ljbade/ttl branch 2 times, most recently from a63c4f5 to b9df5b8 Compare March 30, 2022 04:05
@ljbade ljbade merged commit 2997dd9 into master Mar 30, 2022
@ljbade ljbade deleted the ljbade/ttl branch April 1, 2022 00:52
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