Skip to content

Conversation

@afranchuk
Copy link
Contributor

I think I added it to all the correct applications, but check my work :)

@afranchuk
Copy link
Contributor Author

afranchuk commented Nov 19, 2025

The CI seems to want the file to exist, but maybe we can safely ignore that? The whole point is to land this before the new stuff lands so that there is no hiccup in data collection.

@afranchuk afranchuk force-pushed the add-crashping-library branch from 2fa6c93 to 28be29e Compare November 19, 2025 20:38
@afranchuk afranchuk requested a review from chutten November 19, 2025 20:38
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I thought this will send things under a different app ID?
Why is this now a library?
There already is a ping named crash sent by e.g. firefox_desktop.
Having that re-defined in another library now that's also going under firefox_desktop will lead to complications.

@afranchuk
Copy link
Contributor Author

afranchuk commented Nov 20, 2025

Same app names, different app ids. It is a library used among the existing applications. The crash ping has been wholly moved into the library; the definition will no longer exist in the applications.

The only reason this doesn't remove e.g. the lib-crash pings.yaml definition as a result is because this is landing before the other changes, and I don't want that to stop working in the interim. But the patch removes that file and the one in toolkit/components/crashes which previously defined the crash ping.

@badboy
Copy link
Member

badboy commented Nov 20, 2025

Same app names, different app ids. It is a library used among the existing applications. The crash ping has been wholly moved into the library; the definition will no longer exist in the applications.

The only reason this doesn't remove e.g. the lib-crash pings.yaml definition as a result is because this is landing before the other changes, and I don't want that to stop working in the interim. But the patch removes that file and the one in toolkit/components/crashes which previously defined the crash ping.

your code is passing the application ID "firefox.crashping" to Glean. That's the only thing Glean uses to identify what application this is.
This ultimately results in pings from that crashping application to get submitted to /submit/firefox-crashping/crash/1/$uuid.
Our pipeline will use that name firefox-crashping to look up the schemas and validate the incoming data and then put it into the correct dataset, which in this case would be named firefox_crashping.crash.

"applications" in our repositories.yaml are assumed to all submit their data under the application ID that is specified.
"libraries" are assumed to not have their own application ID and only add pings/metrics, that get send as part of an application.

@afranchuk
Copy link
Contributor Author

afranchuk commented Nov 20, 2025

Ah, I see. I was wondering how that correlates. The application ids are actually going to be derived from the MOZ_APP_NAME: https://phabricator.services.mozilla.com/D267476. However, we can set them by other means. FWIW, this is just a default of the crate, but it can be overridden by the application. It sounds like the crate should not have a default, and the applications should set it to the values the application is already using.

@afranchuk
Copy link
Contributor Author

afranchuk commented Nov 21, 2025

@badboy if there are different application IDs used, but it uses the same glean store, will it use the same client id, or a different one? We actually don't want different client ids here: one of the reasons for this change is so that we can correctly calculate crash incidence among crash reports sent from both Firefox and the crash reporter client (which currently submit with different client ids).

@badboy
Copy link
Member

badboy commented Nov 24, 2025

@badboy if there are different application IDs used, but it uses the same glean store, will it use the same client id, or a different one? We actually don't want different client ids here: one of the reasons for this change is so that we can correctly calculate crash incidence among crash reports sent from both Firefox and the crash reporter client (which currently submit with different client ids).

The application ID isn't stored in the database.
Re-using the same database directory with a different application ID would thus work and send the same data.
However using the same database simultaneously from multiple applications is dangerous. When and how does this client run? Is a Firefox instance active while it runs?
There's also other things we need to consider in this case (e.g. we need to delete/cleanup that crash data with client IDs on opt-out from the user).

Ah, I see. I was wondering how that correlates. The application ids are actually going to be derived from the MOZ_APP_NAME: phabricator.services.mozilla.com/D267476. However, we can set them by other means. FWIW, this is just a default of the crate, but it can be overridden by the application. It sounds like the crate should not have a default, and the applications should set it to the values the application is already using.

By derived you mean it gets that ".crashping" suffix? That means our pipeline will need to know about these new application IDs, or we won't ingest data from it.
Let me talk with my team to figure out what the best way is on this

@afranchuk
Copy link
Contributor Author

The application ID isn't stored in the database. Re-using the same database directory with a different application ID would thus work and send the same data. However using the same database simultaneously from multiple applications is dangerous. When and how does this client run? Is a Firefox instance active while it runs? There's also other things we need to consider in this case (e.g. we need to delete/cleanup that crash data with client IDs on opt-out from the user).

The database will not be used from multiple applications at the same time: Firefox desktop doesn't use the crashping crate at all, it just invokes the crash reporter client in a mode where it sends a ping, and there is a dedicated, unique glean store. As it is now, in both this case and the main-process-crash case (where the crash reporter client is launched), the same application id is used. I was wondering whether, if I changed this to use different application ids between the two cases, it would use the same client id (and it sounds like it would). But maybe it's not worth changing the application ID here: there are other more informative fields if we needed to separate the source of the ping (and, in theory, we shouldn't need to).

By derived you mean it gets that ".crashping" suffix? That means our pipeline will need to know about these new application IDs, or we won't ingest data from it. Let me talk with my team to figure out what the best way is on this

Yes, it gets the suffix and isn't a fixed value. But perhaps it would be clearer and easier to track if it were created with some fixed strings rather than using MOZ_APP_NAME.

@badboy
Copy link
Member

badboy commented Nov 24, 2025

Then let me clarify:

  • The application ID is specified in the configuration passed to Glean.init
  • Glean.init also received a path to a directory. Glean will use this to put its database there and read from the database.
  • The client ID is stored in that database.
  • If two different instances of Glean are pointed to different directories, they will have different databases and there's no sharing of data. They will each have a unique client ID.

@afranchuk
Copy link
Contributor Author

All right, good to know. So, it sounds like I should:

  1. Require passing application IDs to be used from the applications themselves, as a matter of correctness (rather than setting it as part of the crashping crate's logic).
  2. Use the correct application IDs corresponding to the applications already in the repositories.yaml file.

@badboy
Copy link
Member

badboy commented Nov 24, 2025

Let me pull this back into the bug, so the final decision is there. I will also pull in someone from the pipeline team to chime in.

@afranchuk afranchuk force-pushed the add-crashping-library branch from 28be29e to 4687caf Compare December 16, 2025 14:51
@afranchuk
Copy link
Contributor Author

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1989439#c16, once this is merged, we should manually trigger schema generation to confirm this doesn't break anything.

@afranchuk afranchuk requested a review from badboy December 16, 2025 14:53
@badboy badboy force-pushed the add-crashping-library branch from 4687caf to b6b6cd7 Compare December 18, 2025 11:49
@badboy badboy merged commit 3381f9e into mozilla:main Dec 18, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants