Closed
Bug 1263189
Opened 8 years ago
Closed 8 years ago
Skip the new type checks in histogram-tools.py for moz_aggregator & moztelemetry
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
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.
Assignee | ||
Comment 1•8 years ago
|
||
Roberto, can you point me to where its called? And is there a way to test that setup locally?
Flags: needinfo?(rvitillo)
Comment 2•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → gfritzsche
Priority: P2 → P1
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Points: --- → 2
Comment 5•8 years ago
|
||
(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)
Updated•8 years ago
|
Attachment #8740452 -
Flags: feedback?(rvitillo) → feedback+
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+
Assignee | ||
Comment 8•8 years ago
|
||
(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?
Assignee | ||
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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+
Assignee | ||
Comment 11•8 years ago
|
||
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
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2eb4c37c5c
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ee2eb4c37c5c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•