Skip to content

fix(internal): Corrections for DSC metrics #1491

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

Merged
merged 1 commit into from
Sep 22, 2022
Merged

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Sep 22, 2022

Follow-up to #1466 that fixes some outlier conditions:

  • Assume a 0% change vs propagations ratio when there were no changes
  • Log timing metrics even if there was no change (using the start timestamp)
  • Skip changes at the end of every transaction (i.e. event processors)

cc @ale-cota @AbhiPrasad

#skip-changelog

@jan-auer jan-auer requested a review from a team September 22, 2022 10:52
@jan-auer jan-auer self-assigned this Sep 22, 2022
let percentage = ((change as f64) / (total as f64)).min(1.0);
let percentage = match (change, total) {
(0, 0) => 0.0, // 0% indicates no premature changes.
_ => ((change as f64) / (total as f64)).min(1.0) * 100.0,
Copy link
Member

Choose a reason for hiding this comment

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

nit: Doing the x100 here instead of on the presentation layer seems wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, though I have not found a way to do this in Datadog. If you prefer, we can also keep it between 0-1

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine - the name says "percentage", so multiplying by 100 does make the number consistent with the metric name.

@jan-auer jan-auer merged commit d97ff20 into master Sep 22, 2022
@jan-auer jan-auer deleted the fix/tsc-metrics branch September 22, 2022 11:19
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