Scalars.yaml expires: "Never" doesn't work but doesn't error

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: Benjamin Smedberg, Assigned: Dexter)

Tracking

unspecified
mozilla56
Points:
1

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 months ago
I was adding a new scalar and mis-capitalized the expiry in Scalars.yaml:

  plugin_instantiated:
    bug_numbers:
      - 1381591
    description: >-
      The number of plugin instances that were created.
    expires: 'Never' # Jan-2021 but we don't have a version number for that
    kind: uint
    notification_emails:
      - bsmedberg@mozilla.com
    release_channel_collection: opt-out
    record_in_processes:
      - 'main'
      - 'content'

This built correctly but didn't record anything. It would probably be good to add some stricter validation for this field so that the only things that are accepted are "never" lowercase, or a two-digit number.
(Assignee)

Updated

9 months ago
Assignee: nobody → alessio.placitelli
Points: --- → 1
Priority: -- → P1
Whiteboard: [measurement:client]
(Assignee)

Comment 1

9 months ago
Created attachment 8887424 [details] [diff] [review]
bug1381617.patch

Chris, I found that the problem Benjamin mentioned also affects Histogram and events. I refactored the validation code in a shared function, which now consistently checks all the collection primitives.
Attachment #8887424 - Flags: review?(chutten)

Comment 2

9 months ago
Comment on attachment 8887424 [details] [diff] [review]
bug1381617.patch

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

One nit.

::: toolkit/components/telemetry/shared_telemetry_utils.py
@@ +120,5 @@
>  
> +def validate_expiration_version(expiration):
> +    """ Makes sure the expiration version has the expected format.
> +
> +    Allowed examples: "1.0", "20", "300.0a1", "60.0a1", "30.5a1", "never'

mismatched quotes on "never'
Attachment #8887424 - Flags: review?(chutten) → review+
(Assignee)

Comment 3

9 months ago
Created attachment 8887527 [details] [diff] [review]
bug1381617.patch
Attachment #8887424 - Attachment is obsolete: true
Attachment #8887527 - Flags: review+
(Assignee)

Comment 5

9 months ago
Created attachment 8887547 [details] [diff] [review]
bug1381617.patch

Fix the linting problems.
Attachment #8887527 - Attachment is obsolete: true
Attachment #8887547 - Flags: review+
(Assignee)

Comment 6

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/092390ed4c628d13875ddf3f9412e3de2b29a98c
Bug 1381617 - Enable stricter scalar, histogram and events expiration validation. r=chutten

Comment 7

9 months ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/092390ed4c62
Enable stricter scalar, histogram and events expiration validation. r=chutten

Comment 8

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/092390ed4c62
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.