Closed Bug 1263189 Opened 4 years ago Closed 4 years ago

Skip the new type checks in histogram-tools.py for moz_aggregator & moztelemetry

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

The new type checks in bug 1245910 broke the Telemetry dashboard aggregator.
We should probably just make them skippable for that call scenario.
Roberto, can you point me to where its called?
And is there a way to test that setup locally?
Flags: needinfo?(rvitillo)
It's called indirectly from [1]; see also [2]. Some simple tests are at the bottom of [2], mdoglio is going to add a proper test-suite for that package this quarter.

[1] https://github.com/mozilla/python_mozaggregator/blob/master/mozaggregator/db.py#L93
[2] https://github.com/mozilla/python_moztelemetry/blob/master/moztelemetry/histogram.py#L75
Flags: needinfo?(rvitillo)
Assignee: nobody → gfritzsche
Priority: P2 → P1
Summary: Skip the new type checks in histogram-tools.py for moz_aggregator → Skip the new type checks in histogram-tools.py for moz_aggregator & moztelemetry
Comment on attachment 8740452 [details] [diff] [review]
Skip the new type checks in histogram-tools.py on the server-side

This eliminates all errors running |python moztelemetry/histogram.py| locally (except the 101 vs. 102 bucket count error, which i assume is being dealt with elsewhere).

Roberto, Anthony, does that look good to you?
Can you take it through a test-run?
Can you think of any other test-cases we should add at [0]?

0: https://github.com/mozilla/python_moztelemetry/blob/master/moztelemetry/histogram.py#L197
Attachment #8740452 - Flags: feedback?(rvitillo)
Attachment #8740452 - Flags: feedback?(azhang)
Points: --- → 2
(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Roberto, Anthony, does that look good to you?

LGTM

> Can you take it through a test-run?
> Can you think of any other test-cases we should add at [0]?

Mauro, could you please add some more test-cases for the Histogram class to ensure nothing is broken?
Flags: needinfo?(mdoglio)
Attachment #8740452 - Flags: feedback?(rvitillo) → feedback+
Yeah I'll do that as part of bug 1175115.
Flags: needinfo?(mdoglio)
Depends on: 1175115
Comment on attachment 8740452 [details] [diff] [review]
Skip the new type checks in histogram-tools.py on the server-side

Looks good to me! Only thing is the docstring for Histogram is a bit out of date, since it doesn't say what strict_type_checks does.
Attachment #8740452 - Flags: feedback?(azhang) → feedback+
(In reply to Anthony Zhang [:azhang] from comment #7)
> Only thing is the docstring for Histogram is a bit out of
> date, since it doesn't say what strict_type_checks does.

Hm, there is a doc addition on line 95 - do you mean something is missing elsewhere?
Comment on attachment 8740452 [details] [diff] [review]
Skip the new type checks in histogram-tools.py on the server-side

Chris, can you review this?
Attachment #8740452 - Flags: review?(chutten)
Comment on attachment 8740452 [details] [diff] [review]
Skip the new type checks in histogram-tools.py on the server-side

Review of attachment 8740452 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. I look forward to new tests.
Attachment #8740452 - Flags: review?(chutten) → review+
Wrapping this one up to not keep it hanging.
I'll open a follow-up about adding client-side testing for test cases from bug 1175115 etc.
No longer depends on: 1175115
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ee2eb4c37c5c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1271986
You need to log in before you can comment on or make changes to this bug.