1762859 - Consider not recording an error when adding <= 0 to a counter metric, or making the error optional

Status: RESOLVED FIXED

(Data Platform and Tools :: Glean: SDK, defect, P1)

Description

[Beatriz Rizental [:brizental]]

That requires users to use patterns like https://github.com/mozilla-mobile/android-components/blob/71e124a5dbafffb800168ea615fdbc54d7398f50/components/support/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt#L207-L227.

Comment 1

[Chris H-C :chutten]

Like via a new YAML property? non_positive_sample_behaviour: {error_on_zero_or_negative|error_on_negative|ignore} ? (Default: error_on_zero_or_negative)

Comment 2

[Beatriz Rizental [:brizental]]

I did no think too much about it, but I thought maybe an optional argument for add. Needs more consideration though and a YAML property could be a better solution.

Comment 3

[Chris H-C :chutten]

I'm leaning towards

1. Still recording an Invalid State on < 0 because that seems useful
2. No more error on = 0

We couldn't come up with arguments for treating add(0) as an error that stand up to how writing if (value > 0) { Glean.myCategory.myCounter.add(value); } is much worse.

Suppressing on < 0 seems like an argument to finally rev the API to only accept unsigned numbers (forbidding errors is better than reporting errors) which, though I'm in favour of, may require more conversation than just this bug.

And this small change has nice effects on types, making the use of this API more straightforward for unsigned types. (Though we still have an ergonomics squeak with the narrowing conversion from u32 to i32 to make it fit through the udl. Again, we're not talking about scoping in a revision to the API to make it unsigned.)

ni?Travis to make a ruling as Glean SDK Tech Lead.

Comment 4

[Travis Long [:travis_]]

I agree with both 1 & 2.

• Let's keep an error if a negative value is recorded via add()
• Let's remove the error if add(0)

One question I have regards how we now send null in the case a counter is zero. I wonder if we also need to revisit this thinking since it might be surprising if after add(0) is called, the resulting metric is null in the transmitted ping.

Comment 5

[Chris H-C :chutten]

I'm happiest if we keep "should we change our minds about whether 0 is meaningfully different from null or "absent"" out of scope for this bug.

Comment 6

[Travis Long [:travis_]]

Fair enough, we can always file a bug about the treatment of "0" and deal with that in a later iteration, if we even decide to change it.

Comment 7

[BMO Github Automation]

Attachment: [mozilla/glean] bug 1762859 - Silently ignore add(0) on counter and labeled_counter (#2623)

Comment 8

[Chris H-C :chutten]

chutten merged PR #2623: "bug 1762859 - Silently ignore add(0) on counter and labeled_counter" in 6eec4b0.