1557048: Add reason codes to the metrics ping #649

mdboom · 2020-01-14T21:23:10Z

Requires mozilla/glean_parser#157 and mozilla-services/mozilla-pipeline-schemas#484 to be merged first.

This was a real iceberg.

~~Also, no Swift or Python support yet, but thought it worth getting feedback on this much first.~~

Dexterp37

Wow, that was unexpectedly big :o Thanks for tackling it. Leaving a few comments, as I'd like to take another pass after the glean parser changes land.

glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt

glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt

glean-core/pings.yaml

glean-core/metrics.yaml

glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/MetricsPingScheduler.kt

chutten

Do the changes to metrics.yaml and pings.yaml result in any changes to the docs? I was expecting more markdown files in this PR.

mdboom · 2020-02-03T22:21:38Z

@travis79 : Any thoughts on the failure in the iOS integration test? I can't reproduce locally, i.e.:

cd samples/ios/app
carthage bootstrap --platform iOS --cache-builds --configuration Debug --verbose

works for me. I suspect this has something to do with how the integration test build is "special"?

travis79

Left a couple of comments, but I want to take a deeper look tomorrow to build locally and see if I can make some concrete suggestions.

glean-core/ios/Glean/Glean.swift

glean-core/ios/Glean/Metrics/Ping.swift

Dexterp37

First high level view pass, nitpicking on sent vs submitted

CHANGELOG.md

docs/user/pings/index.md

docs/user/pings/metrics.md

glean-core/pings.yaml

glean-core/python/glean/glean.py

glean-core/python/glean/metrics/ping.py

Dexterp37

First high level view pass, nitpicking on sent vs submitted

Dexterp37 · 2020-02-05T11:19:04Z

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt

     */
-    internal fun submitPingsByNameSync(pingNames: List<String>) {
+    internal fun submitPingsByNameSync(pingNames: List<String>, reason: String? = null) {


Please note that pingNames can potentially contain names of different pings (e.g. baseline, events) and we're only passing a single reason: we should probably have a list of pairs or a map, or two lists in order to match specific optional reasons to specific ping names.

This is a good point, and I agree this is a problem. Passing in "parallel lists", one of which is optional, seems kind of a strange API though (at least it's internal API).

I actually feel like it might be better to remove all of the plural submit "pings" in favor of a single submit "ping" on all of these functions. We only use this to submit multiple pings in one place, and we don't even get any parallelism out of thing by batching them up like this. I think that would be a useful simplification.

Thoughts? @Dexterp37, @badboy, @travis79, @brizental ?

I guess, specifically, what I'd propose is to merge this PR as-is with respect to this, and as follow-on work, replace submitPings* with submitPing, which would fix the ambiguity around reason codes.

One other reason we went with submitPings (multiple) was to only call into Rust once.
So far no one has ever complained about FFI calls and given how infrequent and with how small of a list we call it I'm fine with dropping that API

(Users can only submit one ping at a time anyway, through the CustomPing.submit() function, right?)

I'm all for the one at a time, one ping with one reason passed into the function. I also like the idea of this simplifying the underlying FFI function.

@badboy: Yes, through the public API, users can only submit one ping at once. The only place where we submit multiple is on going to background (to send baseline and events).

I'm fine with dropping the plural, but I think it needs to be part of this PR, since this makes #687 buggy (unless we opt for sending the ping on background without a "reason" code.) :-)

Ah, good point about #687. I was mainly just trying to not make this PR any bigger than it already is. But it should be straightforward. I'll take it on.

badboy

I gave this a very rough first pass.
What I would like to see is describing the "why" in the commit message.
Things like highlighting that the reasons are statically defined and become enums as part of the Ping type on the platform side.

CHANGELOG.md

docs/user/pings/metrics.md

glean-core/android/src/main/java/mozilla/telemetry/glean/private/PingType.kt

glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt

glean-core/ios/Glean/Metrics/Ping.swift

codecov-io · 2020-02-05T13:13:11Z

Codecov Report

Merging #649 into master will increase coverage by 12.05%.
The diff coverage is 89.74%.

@@              Coverage Diff              @@
##             master     #649       +/-   ##
=============================================
+ Coverage     75.52%   87.57%   +12.05%     
- Complexity      266      268        +2     
=============================================
  Files            98       59       -39     
  Lines          6191     2971     -3220     
  Branches        751      126      -625     
=============================================
- Hits           4676     2602     -2074     
+ Misses          969      312      -657     
+ Partials        546       57      -489

Impacted Files	Coverage Δ	Complexity Δ
...n/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt	`66.66% <ø> (ø)`	`0 <0> (ø)`	⬇️
...ore/ios/Glean/Scheduler/MetricsPingScheduler.swift	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...ozilla/telemetry/glean/debug/GleanDebugActivity.kt	`75.75% <100%> (ø)`	`7 <0> (ø)`	⬇️
glean-core/ios/Glean/Debug/GleanDebugTools.swift	`80.59% <100%> (ø)`	`0 <0> (ø)`	⬇️
.../telemetry/glean/scheduler/MetricsPingScheduler.kt	`88.04% <80%> (-4.82%)`	`31 <2> (ø)`
...oid/src/main/java/mozilla/telemetry/glean/Glean.kt	`84.83% <81.81%> (-2.81%)`	`1 <0> (ø)`
glean-core/ios/Glean/Glean.swift	`92.72% <86.66%> (-0.49%)`	`0 <0> (ø)`
...n/java/mozilla/telemetry/glean/private/PingType.kt	`78.26% <87.5%> (+3.26%)`	`5 <5> (+2)`	⬆️
glean-core/ios/Glean/Metrics/Ping.swift	`82.75% <90.47%> (+5.83%)`	`0 <0> (ø)`	⬇️
glean-core/src/metrics/timespan.rs
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24257f7...e4a78ae. Read the comment docs.

mdboom · 2020-02-05T23:19:10Z

I think this beast is finally ready for a final review. Apologies for how out-of-hand it became.

EDIT: So, close, but Android tests:

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007fab916ac181, pid=9028, tid=0x00007fab38de5700
#
# JRE version: OpenJDK Runtime Environment (8.0_242-b08) (build 1.8.0_242-b08)
# Java VM: OpenJDK 64-Bit Server VM (25.242-b08 mixed mode linux-amd64 compressed oops)
# Problematic frame:
# C  [libc.so.6+0x15c181]
#
# Core dump written. Default location: /home/circleci/project/glean-core/android/core or core.9028
#
# An error report file with more information is saved as:
# /home/circleci/project/glean-core/android/hs_err_pid9028.log
#
# If you would like to submit a bug report, please visit:
#   http://bugreport.java.com/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#

And with that fun, I'm out for the day.

docs/user/pings/custom.md

Dexterp37

LGTM. What was that bad failure about? What was the fix?

mdboom · 2020-02-06T16:04:11Z

LGTM. What was that bad failure about? What was the fix?

I was never able to see the segfault again. The other intermittent was unrelated to this PR, it turned out, and fixed in #692.

glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt

mdboom · 2020-02-06T19:57:14Z

Good catch. 👍 on fixing that.

…

On Thu, Feb 6, 2020, 1:32 PM Travis Long ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt <#649 (comment)>: > @@ -397,15 +397,15 @@ class LabeledMetricTypeTest { @test(expected = IllegalStateException::class) fun `Test that labeled events are an exception`() { - val eventMetric = EventMetricType<NoExtraKeys>( + val eventMetric = EventMetricType<NoReasonCodes>( Did you mean to change this here? I think NoExtraKeys is correct for an EventMetricType. ------------------------------ In glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt <#649 (comment)>: > disabled = false, category = "telemetry", lifetime = Lifetime.Application, name = "labeled_event_metric", sendInPings = listOf("metrics") ) - val labeledEventMetric = LabeledMetricType<EventMetricType<NoExtraKeys>>( + val labeledEventMetric = LabeledMetricType<EventMetricType<NoReasonCodes>>( I don't think you meant to change this here, I think NoExtraKeys is correct for an EventMetricType. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#649?email_source=notifications&email_token=AAAJLFR2HURLC4P3U3RX3A3RBRJVVA5CNFSM4KGZYVO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUR6EBA#pullrequestreview-354673156>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAJLFWLV54FXUBR4H3MSSLRBRJVVANCNFSM4KGZYVOQ> .

chutten

I mostly skimmed it, and it looked fine. I have Feelings about how the descriptions should be approached, though.

glean-core/pings.yaml

This adds support for sending reason codes along with pings. The reason codes are defined as an enumeration in the pings.yaml file, and only these values are allowed on specific pings. Additionally, this builds on that to add reason codes to the metrics ping.

mdboom requested a review from Dexterp37 January 14, 2020 21:23

mdboom force-pushed the metrics-ping-scheduler-reason branch 3 times, most recently from d3fd05f to bc6429e Compare January 14, 2020 21:27

Dexterp37 reviewed Jan 15, 2020

View reviewed changes

chutten reviewed Jan 15, 2020

View reviewed changes

mdboom force-pushed the metrics-ping-scheduler-reason branch 2 times, most recently from c0c2893 to 24a044a Compare January 23, 2020 13:50

mdboom mentioned this pull request Jan 23, 2020

1611111 - Restore the order of extra arguments in Swift code mozilla/glean_parser#161

Merged

travis79 reviewed Feb 3, 2020

View reviewed changes

glean-core/ios/Glean/Glean.swift Outdated Show resolved Hide resolved

glean-core/ios/Glean/Metrics/Ping.swift Show resolved Hide resolved

mdboom force-pushed the metrics-ping-scheduler-reason branch 2 times, most recently from 0c0158b to 0997bad Compare February 4, 2020 21:44

mdboom marked this pull request as ready for review February 4, 2020 21:44

auto-assign bot requested a review from badboy February 4, 2020 21:44

Dexterp37 reviewed Feb 5, 2020

View reviewed changes

Dexterp37 requested changes Feb 5, 2020

View reviewed changes

Dexterp37 mentioned this pull request Feb 5, 2020

Send a baseline at startup if the previous session ended abruptly #687

Merged

2 tasks

badboy reviewed Feb 5, 2020

View reviewed changes

mdboom force-pushed the metrics-ping-scheduler-reason branch 4 times, most recently from af91250 to 534191d Compare February 5, 2020 23:10

mdboom requested review from badboy, chutten, travis79 and Dexterp37 February 5, 2020 23:18

mdboom force-pushed the metrics-ping-scheduler-reason branch 2 times, most recently from e946fd2 to e4a78ae Compare February 6, 2020 13:38

badboy approved these changes Feb 6, 2020

View reviewed changes

docs/user/pings/custom.md Outdated Show resolved Hide resolved

Dexterp37 approved these changes Feb 6, 2020

View reviewed changes

travis79 requested changes Feb 6, 2020

View reviewed changes

glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt Outdated Show resolved Hide resolved

glean-core/android/src/test/java/mozilla/telemetry/glean/private/LabeledMetricTypeTest.kt Outdated Show resolved Hide resolved

chutten approved these changes Feb 10, 2020

View reviewed changes

glean-core/pings.yaml Outdated Show resolved Hide resolved

mdboom force-pushed the metrics-ping-scheduler-reason branch from 5db0b11 to cf5d803 Compare February 11, 2020 13:22

travis79 approved these changes Feb 11, 2020

View reviewed changes

mdboom added 6 commits February 11, 2020 09:49

Use short form for enums

c05cf78

Suppress compiler warning

136ee3b

swiftlint

f278910

NoReasonCodes -> NoExtraKeys

723380a

Fix descriptions

8595861

mdboom force-pushed the metrics-ping-scheduler-reason branch from bdb369d to 8595861 Compare February 11, 2020 14:49

mdboom merged commit 2e3f59b into mozilla:master Feb 11, 2020

mdboom deleted the metrics-ping-scheduler-reason branch April 14, 2020 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1557048: Add reason codes to the metrics ping #649

1557048: Add reason codes to the metrics ping #649

mdboom commented Jan 14, 2020 •

edited

Loading

Dexterp37 left a comment

chutten left a comment

mdboom commented Feb 3, 2020

travis79 left a comment

Dexterp37 left a comment

Dexterp37 left a comment

Dexterp37 Feb 5, 2020

mdboom Feb 5, 2020

mdboom Feb 5, 2020

badboy Feb 5, 2020

travis79 Feb 5, 2020

mdboom Feb 5, 2020 •

edited

Loading

Dexterp37 Feb 5, 2020

mdboom Feb 5, 2020

badboy left a comment

codecov-io commented Feb 5, 2020 •

edited

Loading

mdboom commented Feb 5, 2020 •

edited

Loading

Dexterp37 left a comment

mdboom commented Feb 6, 2020

mdboom commented Feb 6, 2020 via email

chutten left a comment

1557048: Add reason codes to the metrics ping #649

1557048: Add reason codes to the metrics ping #649

Conversation

mdboom commented Jan 14, 2020 • edited Loading

Dexterp37 left a comment

Choose a reason for hiding this comment

chutten left a comment

Choose a reason for hiding this comment

mdboom commented Feb 3, 2020

travis79 left a comment

Choose a reason for hiding this comment

Dexterp37 left a comment

Choose a reason for hiding this comment

Dexterp37 left a comment

Choose a reason for hiding this comment

Dexterp37 Feb 5, 2020

Choose a reason for hiding this comment

mdboom Feb 5, 2020

Choose a reason for hiding this comment

mdboom Feb 5, 2020

Choose a reason for hiding this comment

badboy Feb 5, 2020

Choose a reason for hiding this comment

travis79 Feb 5, 2020

Choose a reason for hiding this comment

mdboom Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Dexterp37 Feb 5, 2020

Choose a reason for hiding this comment

mdboom Feb 5, 2020

Choose a reason for hiding this comment

badboy left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 5, 2020 • edited Loading

Codecov Report

mdboom commented Feb 5, 2020 • edited Loading

Dexterp37 left a comment

Choose a reason for hiding this comment

mdboom commented Feb 6, 2020

mdboom commented Feb 6, 2020 via email

chutten left a comment

Choose a reason for hiding this comment

mdboom commented Jan 14, 2020 •

edited

Loading

mdboom Feb 5, 2020 •

edited

Loading

codecov-io commented Feb 5, 2020 •

edited

Loading

mdboom commented Feb 5, 2020 •

edited

Loading