-
Notifications
You must be signed in to change notification settings - Fork 4
Add telemetry events #152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: enterprise-main
Are you sure you want to change the base?
Add telemetry events #152
Conversation
…metry sending Changes sendingEnabled() to check AppConstants.MOZ_TELEMETRY_REPORTING instead of Services.telemetry.isOfficialTelemetry. This aligns with the conceptual separation of MOZILLA_OFFICIAL (official Mozilla build) from MOZ_TELEMETRY_REPORTING (telemetry transmission enabled), allowing for more granular control over telemetry behavior. Fixes the issue where "upload is turned off" was incorrectly shown in builds with MOZ_TELEMETRY_REPORTING enabled but MOZILLA_OFFICIAL disabled.
…NG config - Reset telemetry.fog.test.localhost_port to 0 (default) - Use MOZ_TELEMETRY_REPORTING instead of MOZILLA_OFFICIAL for glean upload control - Expose MOZ_TELEMETRY_REPORTING as config variable for build system access
- Create new enterprise ping definition in browser/components/downloads/pings.yaml - Move both file_downloaded and security_download_completed events to enterprise ping - Consolidate all download telemetry events in browser component metrics.yaml - Register enterprise ping in glean metrics_index.py for Firefox Desktop - Add GleanPings.enterprise.submit() calls to actually send the ping - Remove duplicate telemetry event definitions from toolkit component This separates enterprise security telemetry from the general events ping, providing better data organization and privacy controls.
PathUtils is available as a WebIDL global binding in Firefox, not as an ES module. Removed the ChromeUtils.importESModule call and fallback code since PathUtils is always available in this context.
797e0e6 to
c2bbf23
Compare
c2bbf23 to
0bd4adb
Compare
| ); | ||
|
|
||
| // DEBUG: Enhanced logging for download telemetry debugging | ||
| console.log(`[DownloadsCommon] _notifyDownloadEvent called with aType: ${aType}, download: ${download ? 'present' : 'null'}, succeeded: ${download?.succeeded}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the console.log debugging as part of the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that can all be reduced to the minimum, a lot of this was tracing to see where things were going.
browser/components/downloads/DownloadsTelemetry.enterprise.sys.mjs
Outdated
Show resolved
Hide resolved
| The download source URL. Collection level configurable via enterprise | ||
| policy DownloadTelemetryUrlLogging: "full" (default), "domain", or "none". | ||
| type: string | ||
| security_download_completed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gcp What's the difference between the two metrics file_downloaded and security_download_completed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name.
...which allowed me to see which Firefox code was actually triggering the submission when I was debugging this.
We may not need two, but I vaguely remember that one of these actually triggers when initiating a download, and the other when it completes. In that case, we can keep them with some renaming.
| --- | ||
| $schema: moz://mozilla.org/schemas/glean/pings/2-0-0 | ||
|
|
||
| enterprise: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file belong somewhere else? Perhaps browser/components/enterprise (despite there not being any metrics there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
| bool IsValidDestination(std::string aHost) { | ||
| static const std::string kValidDestinations[] = { | ||
| "localhost", | ||
| "127.0.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove (was it just for debugging/testing)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
| if cfg!(mock) { | ||
| "https://incoming.glean.example.com".to_string() | ||
| } else { | ||
| std::env::var("TELEMETRY_ENDPOINT") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to change with the updates from #158 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this can all go in favor of @fiji-flo's patch.
| #include "nsStringFwd.h" | ||
|
|
||
| #define TELEMETRY_BASE_URL "https://incoming.telemetry.mozilla.org/submit" | ||
| #define TELEMETRY_BASE_URL_DEFAULT "https://incoming.telemetry.mozilla.org/submit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
| }) | ||
| ); | ||
| #ifdef MOZ_ENTERPRISE | ||
| GleanPings.enterprise.submit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put behind pref check once added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm not sure if there's a meaningful way to put this metric behind a pref check. The metric already exists and is sent regardless. Adding it to the enterprise ping seems to be a compile time decision. Avoiding submitting the enterprise ping here doesn't prevent it from being recorded and sent when the next enterprise ping is triggered. So just leave this as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate the ping, record it twice, make the enterprise recording conditional.
We could have the console pick up the regular ping for this, but as said, "event" pings are not "live" so this may not be what we want.
Edit: Wait, this doesn't make sense, does it? We already get this through normal Glean Telemetry and it's on by default, so we can just leave this here to get the "live" update.
toolkit/xre/nsAppRunner.cpp
Outdated
| // it as a user pref so the rest of the startup code reads the | ||
| // overridden value. This allows `TELEMETRY_ENDPOINT` to control the | ||
| // `toolkit.telemetry.server` preference at runtime. | ||
| if (const char* telemetryEnv = PR_GetEnv("TELEMETRY_ENDPOINT")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah all the TELEMETRY_ENDPOINT logic can go now.
| ); | ||
|
|
||
| // DEBUG: Enhanced logging for download telemetry debugging | ||
| console.log(`[DownloadsCommon] _notifyDownloadEvent called with aType: ${aType}, download: ${download ? 'present' : 'null'}, succeeded: ${download?.succeeded}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that can all be reduced to the minimum, a lot of this was tracing to see where things were going.
| * | ||
| * Configuration Options: | ||
| * - Enabled (boolean): Enable/disable download telemetry collection | ||
| * - UrlLogging (string): URL logging level with values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this wants FileLogging as well, where there is an intermediate level that just logs the extension and MIME type.
| description: > | ||
| The MIME type of the downloaded file as reported by the server. | ||
| type: string | ||
| file_size: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderfully inconsistent with the other ping 😢
| --- | ||
| $schema: moz://mozilla.org/schemas/glean/pings/2-0-0 | ||
|
|
||
| enterprise: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
| { | ||
| title: "Structured Ingestion telemetry server endpoint", | ||
| value: "https://incoming.telemetry.mozilla.org/submit", | ||
| getValue: () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rip it out then!
| fn should_ohttp_upload(upload_request: &PingUploadRequest) -> bool { | ||
| !upload_request.body_has_info_sections | ||
| // Disabled OHTTP uploads for local testing | ||
| false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably from my WIP. I think we want to drop all changes in these files and use @fiji-flo's patched viaduct uploader.
| try { | ||
| const sourceUrl = this._processSourceUrl(this.activeURI); | ||
| const topSourceUrl = this._processSourceUrl(this.topCurrentURI); | ||
| Glean.printing.pagePrinted.record({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Page count sounds like something an admin wants to know.
| "MOZ_JXL", | ||
| "MOZ_SYSTEM_POLICIES", | ||
| "MOZ_SELECTABLE_PROFILES", | ||
| "MOZ_ENTERPRISE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this now that other code defined MOZ_ENTERPRISE already?
| }) | ||
| ); | ||
| #ifdef MOZ_ENTERPRISE | ||
| GleanPings.enterprise.submit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate the ping, record it twice, make the enterprise recording conditional.
We could have the console pick up the regular ping for this, but as said, "event" pings are not "live" so this may not be what we want.
Edit: Wait, this doesn't make sense, does it? We already get this through normal Glean Telemetry and it's on by default, so we can just leave this here to get the "live" update.
| "browser.", | ||
| "datareporting.policy.", | ||
| "dom.", | ||
| "enterprise.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonathanmendez This one is confusing me, I don't think this patch adds any preferences under that prefix?
| include_client_id: false | ||
| send_if_empty: false | ||
| metadata: | ||
| include_info_sections: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fiji-flo @jonathanmendez This adds a bunch of junk to the pings:
{
"client_info": {
"app_build": "20251123004502",
"app_channel": "default",
"app_display_version": "147.0a1",
"architecture": "aarch64",
"build_date": "1970-01-01T00:00:00+00:00",
"first_run_date": "2025-11-06+01:00",
"locale": "en-US",
"os": "Darwin",
"os_version": "25.1.0",
"telemetry_sdk_build": "66.1.1"
},
"events": [
{
"category": "downloads",
"extra": {
"extension": "exe",
"filename": "setupDeepSjengWC2008(1).exe",
"glean_timestamp": "1763857152212",
"is_private": "false",
"mime_type": "application/octet-stream",
"size_bytes": "1042621",
"source_url": "https://sjeng.org/dl/setupDeepSjengWC2008.exe"
},
"name": "download_completed",
"timestamp": 0
}
],
"ping_info": {
"end_time": "2025-11-23T01:19:12.214+01:00",
"experiments": {
"ai-chatbot-page-summarization-mvp-treatment-a-callout-badge-rollout-v2": {
"branch": "treatment-a-callout-badge",
"extra": {
"type": "nimbus-rollout"
}
},
"fx-accounts-ping-release-rollout-2": {
"branch": "control",
"extra": {
"type": "nimbus-rollout"
}
},
"smart-tab-groups-rollout-beta": {
"branch": "smart-tab-groups",
"extra": {
"type": "nimbus-rollout"
}
}
},
"seq": 16,
"start_time": "2025-11-23T01:19:11.000+01:00"
}
}
Specifically client_info and ping_info. I mean client_info isn't entirely useless but we expect to already have this as part of posture, and this data is now in every ping. Can't we fix the console to handle the absence of these?
- Move pings.yaml from browser/components/downloads to browser/components/enterprisepolicies (it's a shared enterprise ping used by multiple components) - Update metrics_index.py to reflect new pings.yaml location - Align download metric field naming (extension, size_bytes) across both metrics - Add data_sensitivity: highly_sensitive to enterprise metrics for consistency All other review comments were already addressed in earlier rebased commits.
Uh oh!
There was an error while loading. Please reload this page.