Skip to content
This repository has been archived by the owner on May 19, 2022. It is now read-only.

fix non-positive durations for metrics #15

Merged
merged 2 commits into from
Jun 12, 2020

Conversation

malafeev
Copy link
Contributor

@malafeev malafeev commented May 21, 2020

  • deleted unused rc/main/java/com/lightstep/tracer/metrics/SafeMetrics.java and corresponding test class
  • duration of first report is 0 therefore I drop it. It happens not only on start up because we kill metrics reporting thread if there are no traces during timeout and then start again if there are. So when we again start thread duration of first report is 0, so drop it. This should fix non-positive duration.

Signed-off-by: Sergei Malafeev <sergeymalafeev@gmail.com>
@andrewhsu
Copy link

Talked to @malafeev, could use test case, comments in the commit, and possibly a check for positive value.

Signed-off-by: Sergei Malafeev <sergeymalafeev@gmail.com>
@carlosalberto
Copy link
Contributor

Overall it looks great. Only two things I'd like to be verified:

  1. Not sending the first report ever used to break the metrics tests. I saw you changed the metrics counts in a few tests, to correct this. Would you mind running the tests many times to verify they are still correct? (the tests are rather flaky, but still).

  2. I'd like to see this tested against the actual backend. As you don't have experience using the RCA metrics on the backend, lets coordinate offline to make that happen ;)

@malafeev
Copy link
Contributor Author

  1. yes, I ran it many times locally, looks stable. But all these sleeps can potentially produce false positives.
  2. I didn't see metrics in UI, probably not enough permissions

@malafeev
Copy link
Contributor Author

I see metrics :)
Screen Shot 2020-06-12 at 14 58 20

@carlosalberto carlosalberto merged commit a9f3b24 into lightstep:master Jun 12, 2020
@carlosalberto
Copy link
Contributor

Let's give it a run!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants