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

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

The new type checks in bug 1245910 broke the Telemetry dashboard aggregator.
We should probably just make them skippable for that call scenario.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Roberto, can you point me to where its called?
And is there a way to test that setup locally?

Comment 2

[Roberto Agostino Vitillo (: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

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Skip the new type checks in histogram-tools.py on the server-side

Comment 4

[Georg Fritzsche [:gfritzsche]]

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

Comment 5

[Roberto Agostino Vitillo (:rvitillo)]

(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?

Comment 6

[Mauro Doglio [:mdoglio]]

Yeah I'll do that as part of bug 1175115.

Comment 7

[Anthony Zhang [:azhang] (last day at Mozilla: 2016-04-29)]

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.

Comment 8

[Georg Fritzsche [:gfritzsche]]

(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 9

[Georg Fritzsche [:gfritzsche]]

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?

Comment 10

[Chris H-C :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.

Comment 11

[Georg Fritzsche [:gfritzsche]]

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2eb4c37c5c

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee2eb4c37c5c