Closed Bug 1550690 Opened 6 years ago Closed 5 years ago

Consider moving metrics.yaml "bugs" property to use urls

Categories

(Data Platform and Tools :: Glean: SDK, enhancement, P3)

Product:
Data Platform and Tools
Component:
Glean: SDK
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gfritzsche, Unassigned)

Details

(Whiteboard: [telemetry:glean-rs:backlog])

Currently we use "bug numbers" for listing out GH issues or Bugzilla bug numbers.

That leads to not very useful entries like this:

  # ...
  bugs:
  - 800

Off-hand i can't tell what that means & it's programmatic interpretation is ambitious.

What if we change this to require strings or URLs:

  # ...
  - https://github.com/mozilla-mobile/fenix/issues/800

For the record, this is how it's currently defined:

A list of bugs (e.g. Bugzilla and Github) that are relevant to this metric, e.g., tracking its original implementation or later changes to it.

If a number, it is an ID to an issue in the default tracker (e.g. Mozilla’s Bugzilla instance). If a string, it must be a URI to a bug page in a tracker.

On second look, I see that both kinds of numbers are mixed in that same file. Yuck.

I suggest we stage this as follows to avoid getting into a broken state:

  1. File pull requests against all glean-using things to use full URLs for bugs

  2. Turn off the use of integers in the schema and glean_parser

Stepping back, I suppose the deeper question is "What is the purpose of this field?" If it's to have a permanent link to discussion about what lead to the data collection, a Github issue is probably not ideal, and encouraging the use of Bugzilla (which has more of the properties of a "permalink") is probably better.

Priority: -- → P2

If it's for linkage to the Data Collection Review, the review process requires a bugzilla bug anyway (if that makes a difference).

ni?liuche -- is this your understanding as well?

Flags: needinfo?(liuche)

I agree that the purpose of this field is providing any context on this probe, which would likely live in the bug discussion. However, there are certainly several products that don't have any Bugzilla presence (and are Github only), so saying "link only to bugzilla issue numbers" won't really work (and would probably just link back to the Github issue anyway).

I don't see movement to bugzilla-only right now, so while I think urls in this yaml file look kind of gross, I think it makes sense to switch to url-only, just so a) we don't mix numbers between the different issue trackers, and b) it's generic support for the future. (Accessing the bugzilla bug still requires you to search for that bug number anyway, right? So a url is at least a direct link to the bug context.)

Flags: needinfo?(liuche)

Since version 1.9.0 the glean-parser warns that URLs should be used instead of bug numbers in the "bugs" field. This has been around for about two quarters.

Mike, should we do a PSA email and start breaking builds?

Flags: needinfo?(mdroettboom)
Priority: P2 → P3
Whiteboard: [telemetry:glean-rs:backlog]

As one note: we can't change the schema that glean-parser uses, as it's also used to parse old versions of metrics.yaml files.
We would need to extend the parser to have different modes (relaxed, old-supporting mode for probe-scraper, strict & rejecting invalid stuff for projects).

In addition the linter currently can't differentiate between warnings and errors, so we would need to build that functionality first too

Why do we need to have different modes? Whoever wants to parse historical YAML can obtain a historical glean_parser, no? We version these things, don't we?

Sure, that would be a third option, but that's not how it currently works (and might be even trickier to fit into probe-scraper ... now it not only needs to load one glean_parser, but also need to find the right glean_parser for each file in each revision)

AFAIK, all of our users have already made these updates, so I'd be in favor of the PSA + making it an error.

Flags: needinfo?(mdroettboom)

Bug numbers are now an error, so closing this bug.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.