Closed Bug 1601270 Opened 6 years ago Closed 6 years ago

Restrict ping names to be kebab-case (again)

Categories

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

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: janerik, Assigned: janerik)

References

Details

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

klukas informed me that we want to disallow underscores in ping names.

We previously attempted to do just that (https://github.com/mozilla/glean_parser/pull/90), but reverted it because we weren't ready for the impact.
The follow-up wasn't merged (https://github.com/mozilla/glean_parser/pull/94).

Now that we have deletion_request this comes up again and we need to act on it.

The pipeline can handle it.
The probe-scraper might start rejecting older pings if we switch it over.
klukas can help in determining impact and necessary changes on their end.

From https://github.com/mozilla/glean_parser/pull/90#issuecomment-525773761

Just spoke with Frank and it sounds like we pretty much have a way forward for this:

  • Merge this change and deploy a new version of glean_parser
  • Update MPS to include both the old snake_case names and the new kebab-case names for sync pings
  • Update MSG to convert snake-case ping names from probe info service to kebab-case (to handle the interim before fenix gets the updated names)
  • PR android-components to take the new glean_parser and update the sync ping names to kebab-case
  • Fenix gets the updated android-components and should otherwise not notice any change

What is MPS?
What is MSG?

Flags: needinfo?(jklukas)

I still don't fully understand why we need this, especially given the impact this will have, now that we have multiple pings with an underscore.
:klukas, can you expand on why the change is necessary?

Assignee: nobody → jrediger
Priority: P3 → P1
Whiteboard: [telemetry:glean-rs:m?] → [telemetry:glean-rs:m11]

MPS refers to the mozilla-pipeline-schemas repo and MSG refers to the mozilla-schemas-generator repo.

I still don't fully understand why we need this, especially given the impact this will have, now that we have multiple pings with an underscore.

I am currently only aware of two existing ping names with underscores (the two _sync pings) and this new deletion_request ping. Are there others?

I am willing to prioritize working on the changes to MPS and MGS so that we can get this codified and avoid this continuing to cause pain in the future.

:klukas, can you expand on why the change is necessary?

We never properly intended to support pings with an underscore. The telemetry docs have specified for a long time that document namespace and type should be limited to characters [a-z-] [0]. There was some miscommunication that led to us allowing the two _sync ping names with underscores into mozilla-pipeline-schemas.

I would not call this change necessary in that we can technically build and maintain support for ping names with underscores. But it is important for consistency and being able to reason about the system end-to-end. Continuing to support both options will lead to ping authors making stylistic decisions, and thus it becomes difficult for users to predict the casing of ping names. It also leads to more complicated logic in the pipeline that we would need to support long-term and increases the possibility of error when we do operational maintenance like backfilling between sources.

[0] https://docs.telemetry.mozilla.org/cookbooks/new_ping.html#choose-a-namespace-and-doctype

Flags: needinfo?(jklukas)

I had a chat with :klukas about the steps forward.

  1. Land changes to glean_parser (based on the initial PR): Restrict ping names to 30 alphanumeric + dash characters. Additionally allow: deletion_request, history_sync, bookmarks_sync.
    • Release a new version glean_parser.
  2. Update used glean_parser version in the probe-scraper to the new version
  3. Ensure the schema generator can handle both dashes and underscores and generates the same table names (e.g. deletion_request and deletion-request are considered to be the same
  4. Update glean_parser in Glean
    • At the same time also rename deletion_request to deletion-request while we're at it
  5. Rename history_sync and bookmarks_sync to their dashed versions
  6. Land new Glean and new sync in Fenix (and other applications)
  7. Double-check documentation on dtmo and the Glean SDK book agree on naming.

Needed to fix the codegen in glean_parser, but now the above patches should be good and on their way.

Renames for a-c are up here: https://github.com/mozilla-mobile/android-components/pull/5306

:klukas, just as a safety check, we're good to land this rename?

Flags: needinfo?(jklukas)

PR up here: https://github.com/MozillaReality/FirefoxReality/pull/2478
Caution: this can't land right now as it requires upgrades to android-components first.

(In reply to Jan-Erik Rediger [:janerik] from comment #9)

Renames for a-c are up here: https://github.com/mozilla-mobile/android-components/pull/5306

:klukas, just as a safety check, we're good to land this rename?

Yes, we have landed the necessary change in mozilla-schema-generator. So the schema generation and ingestion pipelines are ready for this change.

Flags: needinfo?(jklukas)

filed follow up bugs, so this can be considered done.

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