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

Python: Implement timing distribution #762

Merged
merged 5 commits into from
Mar 9, 2020

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Mar 6, 2020

No description provided.

@auto-assign auto-assign bot requested a review from badboy March 6, 2020 20:30
@mdboom mdboom force-pushed the python-timing-distribution branch from e303320 to 0bf975d Compare March 6, 2020 20:31
@mdboom mdboom force-pushed the python-timing-distribution branch from 0bf975d to 635417b Compare March 6, 2020 20:32

There are test APIs available too. For convenience, properties `sum` and `count` are exposed to facilitate validating that data was recorded correctly.

Continuing the `page_load` example above, at this point the metric should have a `sum == 11` and a `count == 2`:
Copy link
Member

Choose a reason for hiding this comment

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

Re-reading this now I somehow find it confusing that "it should have" values.
This is a general problem with the example though and repeated across all languages.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. We can handle updating all of the language examples at once as part of that bug.

assert 2 == snapshot.count

# Was an error recorded?
assert 1 == metrics.pages.page_load.test_get_num_recorded_errors(
Copy link
Member

Choose a reason for hiding this comment

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

See, it's nowhere clear that above example causes an error.

Copy link
Member

Choose a reason for hiding this comment

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

Let me file a bug

Copy link
Member

Choose a reason for hiding this comment

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

@@ -76,7 +76,7 @@ class TimingDistributionMetricType internal constructor(
return null
}

// The Rust code for [stopAndAccumulate] runs async and we need to use the same clock for start and stop.
// The Rust code for [start] runs async and we need to use the same clock for start and stop.
Copy link
Member

Choose a reason for hiding this comment

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

Woops, good catch

Copy link
Member

Choose a reason for hiding this comment

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

ooooh wait, the comment was correct, just not super clear.

start does run sync (there's no dispatcher in use).
But stop runs async (we don't care about its return value) and THAT's why we need to use getElapsedTimeNanos in both because we need to use the same clock source.
I'm all for extending the comment, but the original was already more correct than after your change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok -- I'll update this to be both correct and clear.

glean-core/python/glean/metrics/timing_distribution.py Outdated Show resolved Hide resolved
glean-core/python/glean/metrics/timing_distribution.py Outdated Show resolved Hide resolved
@mdboom mdboom requested a review from badboy March 9, 2020 12:01
@mdboom mdboom force-pushed the python-timing-distribution branch from 7543f89 to cd8d8fd Compare March 9, 2020 12:44
Co-Authored-By: Jan-Erik Rediger <badboy@archlinux.us>
@mdboom mdboom merged commit dca46ba into mozilla:master Mar 9, 2020
@mdboom mdboom deleted the python-timing-distribution branch April 14, 2020 19:09
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