Skip to content

Telemetry: Add application telemetry settings dialog#2622

Merged
ArturoManzoli merged 4 commits into
bluerobotics:masterfrom
ArturoManzoli:2573-add-telemetry-agreement
May 8, 2026
Merged

Telemetry: Add application telemetry settings dialog#2622
ArturoManzoli merged 4 commits into
bluerobotics:masterfrom
ArturoManzoli:2573-add-telemetry-agreement

Conversation

@ArturoManzoli
Copy link
Copy Markdown
Contributor

@ArturoManzoli ArturoManzoli commented Apr 23, 2026

  • Added a dialog informing the user about the basic data sharing;
  • User can opt-out from sharing hardware information.
Screenshare.-.2026-05-08.11_42_03.AM.mp4

Closes #2622

@github-actions
Copy link
Copy Markdown

Automated PR Review (Claude)

0. Summary

Verdict: MINOR SUGGESTIONS

Minor items to address: 1.1, 1.2, 2.1, 6.1, 6.2.

This PR replaces the single boolean usage-statistics toggle with a tiered, cumulative data-privacy consent model (None → Errors → Usage → Vehicle → Device). It introduces a new DataPrivacyModal component with a vertical slider UI, a new telemetryPrivacy Pinia store, per-event privacy-level gating in the event tracker, a migration for the legacy toggle, a persistent snackbar option, and multiple UI entry-points (About dialog, Settings → General, first-launch onboarding).


1. Correctness & Implementation Bugs

1.1 (major) — src/main.ts: The PR changes the settings-manager import from import { settingsManager } from '@/libs/settings-management' to a bare side-effect import import '@/libs/settings-management'. This works for triggering the module's side effects, but the base code at line 50 uses settingsManager.getKeyValue(...) directly. The PR removes that usage, so it's fine for this PR, but removing the named export from the import line means any future code in main.ts that needs settingsManager would need to re-add it. More importantly, the getEffectiveTelemetryPrivacyLevel function (used on the very next line after the removed reference) internally calls settingsManager.getKeyValue, which depends on the settings manager singleton being initialized. Because the bare import still triggers module evaluation (and hence the new SettingsManager() call at the bottom of settings-management.ts), this works correctly in practice. No bug, but worth noting the implicit dependency.

1.2 (minor) — src/stores/omniscientLogger.ts: The Ping event now gates vehicle context behind privacyStore.allowsVehicleContext, which is a reactive computed. However, the EventTracker.enableEventTracking flag is set once in the constructor based on the privacy level at startup time. If a user starts the app without confirming privacy (level = None), the EventTracker constructor sets enableEventTracking = false and never initializes PostHog or the queue. After the user confirms a choice (even to "Device"), the eventTracker.capture() calls will still return early because enableEventTracking remains false and eventTrackingQueue is undefined. This means privacy changes made during the current session take effect only on the next app launch, not immediately. The confirm-snackbar text ("Some changes apply on the next app start") partially communicates this, but it might surprise users that all telemetry changes require a restart since the EventTracker is a singleton constructed once. This is an existing architectural limitation, not a regression — but worth documenting more explicitly.

1.3 (nit) — src/components/DataPrivacyModal.vue:406: initialSelectedLevel() casts settingsManager.getKeyValue(...) with as TelemetryDataPrivacyLevel. If the persisted value somehow falls outside the enum range (e.g. due to a future version change or storage corruption), the cast silently passes. A bounds check / Object.values(TelemetryDataPrivacyLevel).includes(...) would be more defensive, but this is low risk.


2. AGENTS.md Adherence

2.1 (minor) — JSDoc completeness (Rule 4): The new useTelemetryDataPrivacyStore Pinia store (line 72 of telemetryPrivacy.ts) lacks a JSDoc comment on the store definition itself. Per AGENTS.md Rule 4, all function declarations should have JSDoc. The defineStore callback is a function expression passed to defineStore, so the eslint rule may not enforce it, but a short description would be consistent with the project's convention.

2.2 (nit) — Comment policy (explain "why" not "what"): Several new comments are informative and explain "why" well (e.g. the migration JSDoc). The comment // Hide the settings menu while a top-level modal is shown on top of it... in App.vue is good — it explains "why."

2.3 (nit) — cockpit- prefix for new storage keys (Rule): Both new keys cockpit-telemetry-data-privacy-level and cockpit-telemetry-data-privacy-choice-made correctly use the cockpit- prefix. ✓

2.4 (nit) — No new dependencies added.


3. Security

3.1 No obfuscated or intentionally unreadable code found.

3.2 No suspicious base64/hex/encoded blobs or binary-like strings.

3.3 No hidden Unicode, zero-width characters, RTL overrides, or homoglyph attacks detected.

3.4 No unexpected network calls. The telemetry endpoints (PostHog, Sentry) are pre-existing. The new telemetryDataPrivacyDocsUrl points to https://blueos.cloud/cockpit/docs/latest/usage/privacy, which is the project's own documentation domain. No exfiltration patterns.

3.5 No changes to build scripts, postinstall hooks, CI workflows, Dockerfiles, or Electron main-process code.

3.6 No new use of eval, Function(), v-html, or credential handling. No weakened CORS/CSP.

3.7 No new dependencies added. No package.json changes in this PR.

3.8 No patterns suggesting malicious behavior. The PR is a straightforward privacy-controls improvement that gates telemetry behind explicit user consent.


4. Performance

4.1 (nit) — src/stores/omniscientLogger.ts: The setInterval Ping callback (every 5 minutes) now reads privacyStore.allowsVehicleContext on every tick. This is a lightweight computed read, so no meaningful perf concern.

No other performance issues identified. No new subscriptions are left uncleared; no heavy deps added.


5. UI / UX

5.1 (nit) — The modal is non-dismissible on first launch (no "Cancel" button visible, persistent prevents clicking outside). This is intentional and correctly implemented. The user must explicitly confirm a privacy level before any telemetry is sent. Good UX.

5.2 (nit) — Keyboard accessibility: The tier dots use <button> elements with aria-label and aria-pressed, which is good. The tier text rows use @click on <div> elements without role="button" or tabindex, making them keyboard-inaccessible. Consider adding role="button" tabindex="0" @keydown.enter="selectedLevel = tier.level" to these rows for accessibility parity.

5.3 (nit) — Hardcoded color values like #4fa483, #7ad1aa, #5b6770 are used extensively in the template. These don't appear to reference the existing theme system (Vuetify theme or UIGlassEffect). This is consistent with how other modal components in the codebase use hardcoded colors, so not a departure from existing patterns.


6. Code Quality & Style

6.1 (minor) — src/composables/snackbar.ts: The exported closeSnackbar function (line 43 in the diff) is exported both as a standalone named export and as part of the useSnackbar() composable return value. The Snackbar.vue component imports it as a standalone export (import { closeSnackbar as removeSnackbar }), while other callers use the composable. This dual-export pattern works but creates two paths to the same function. Consider whether the standalone export is needed or if the component should use the composable instead (though the component may not use setup() in a way that makes the composable ergonomic — the current approach is acceptable).

6.2 (minor) — src/components/DataPrivacyModal.vue: The tiers array is declared as a plain const (not readonly / as const). Since it's a static configuration that never changes, marking it as const would provide stricter type inference and signal immutability. This is minor.

6.3 (nit) — src/stores/telemetryPrivacy.ts:67: readonly(level) and readonly(choiceMade) — good use of Vue's readonly to enforce the invariant that these can only be mutated through confirmLevel. Clean pattern.

6.4 (nit) — The PrivacyTier interface and tiers array in DataPrivacyModal.vue are well-structured and self-documenting.


7. Tests

7.1 (minor) — No tests are added for the new telemetryPrivacy store, the migration logic, or the DataPrivacyModal component. The store has meaningful logic (effective level computation, choice-made gating) that would benefit from unit tests. The migration function migrateLegacyTelemetryToggleToPrivacyLevel has multiple branches that are testable. This is consistent with the project's current low test coverage, but worth noting for follow-up.


8. Documentation

8.1 (nit) — The telemetryDataPrivacyDocsUrl points to https://blueos.cloud/cockpit/docs/latest/usage/privacy. If this page doesn't exist yet, the "Learn more" link will 404. Ensure the documentation is published before or alongside this PR.

8.2 (nit) — No README update needed — this feature works identically in both Lite and Standalone builds, so no feature-parity table entry is required per AGENTS.md.


9. Nitpicks / Optional

9.1 (nit) — src/components/DataPrivacyModal.vue:269: The Confirm button uses class="bg-[#FFFFFF33] text-white". Consider using Vuetify's color prop for consistency, though this matches patterns seen elsewhere in the codebase.

9.2 (nit) — src/utils/migrations.ts: The migration reads localStorage.getItem(legacyKey) directly rather than going through settingsManager.getKeyValue(). This is correct — the legacy key was stored via useStorage (VueUse), which writes directly to localStorage, not through the settings manager. The approach is sound.

9.3 (nit) — The PR description has a typo: "Coses #2573" should be "Closes #2573".

9.4 (nit) — src/stores/telemetryPrivacy.ts: The enum values are integers (0–4). If the enum is ever reordered or values inserted between existing ones, the persisted integer values would silently map to wrong levels. A comment noting that the numeric values are part of the storage contract would help future maintainers. The current ordering is logical and unlikely to change, so this is very low risk.


Generated by Claude. This is advisory; a human reviewer must still approve.

Copy link
Copy Markdown
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

Some points for us to discuss:

  • There should be a minimum telemetry level, where we are at least informed about the decision of the user on disabling telemetry.
  • This is killing inclusive our current number-of-active-users data, leaving us blind.
  • This opt-out non-dismissible approach is going to hit us hard. I don't believe we should go that way.

Copy link
Copy Markdown
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

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

Great start - thanks!! :D

A few minor comments:

  1. The window feels wider than it needs to be, especially for the text size
  2. It feels like a lot to take in, which I think is partly because of how small and spread out the text is
    • but is maybe also a sign that we need to shorten some of the descriptions, not sure...
  3. I think the "unapplied grey" should get used on any unselected section titles and data types, so it's clearer what is and isn't included:
Image

There should be a minimum telemetry level, where we are at least informed about the decision of the user on disabling telemetry.

If we do this we would need to change the "No application telemetry" description, so it's not lying. First thought is "All we know is someone chose not to share", but we may be able to come up with better/nicer wording. The title should maybe be changed to "No ongoing telemetry".


Some minor suggestions:

Comment thread src/components/DataPrivacyModal.vue
Comment thread src/components/DataPrivacyModal.vue Outdated
max-width="1020"
variant="text-only"
>
<template #title><div class="flex items-center w-full justify-center">Data Privacy</div></template>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<template #title><div class="flex items-center w-full justify-center">Data Privacy</div></template>
<template #title><div class="flex items-center w-full justify-center">Help Improve Cockpit!</div></template>

This is debatable, but I feel like it better communicates our intent / what we want from users.

Comment thread src/components/DataPrivacyModal.vue Outdated
Comment on lines +13 to +15
Cockpit can share anonymous information with the Blue Robotics development team to help us understand how
the application is being used. The more you share, the better we can target our development and testing
efforts to your needs - but you stay in control of what is included.
Copy link
Copy Markdown
Contributor

@ES-Alexander ES-Alexander Apr 24, 2026

Choose a reason for hiding this comment

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

I'm a bit concerned users won't want to read this much text, but I also don't want to cut any of it out...

A very minor change:

Suggested change
Cockpit can share anonymous information with the Blue Robotics development team to help us understand how
the application is being used. The more you share, the better we can target our development and testing
efforts to your needs - but you stay in control of what is included.
Cockpit can share anonymous information with the Blue Robotics development team to help us understand how
the application is being used. The more you share, the better we can target our development and testing
efforts to your needs - but you're in control of what is included.

Comment thread src/components/DataPrivacyModal.vue Outdated
@click="showExamples = !showExamples"
>
<v-icon size="14">{{ showExamples ? 'mdi-chevron-down' : 'mdi-chevron-right' }}</v-icon>
{{ showExamples ? 'Hide example data' : 'Show example data' }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally the example data would include the actual values from the user's system (possibly in a separate column?), but that seems ok to leave for a later implementation if it's non-negligible.

Comment thread src/components/DataPrivacyModal.vue Outdated
{
level: TelemetryDataPrivacyLevel.Device,
title: 'Device information',
subtitle: 'CPU/GPU type, available memory, screen size, locale',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
subtitle: 'CPU/GPU type, available memory, screen size, locale',
subtitle: 'CPU+GPU type, available memory, screen size, locale',

Comment thread src/components/DataPrivacyModal.vue Outdated
negative: `We'll rely on internal testing and support requests to find bugs.`,
examples: [
'Uncaught exceptions and stack traces',
'Performance traces',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel like this belongs in the Usage statistics, since it's related to memory leaks and isn't directly tied to errors. Otherwise we should reword the categories and positive/negative points.

@ES-Alexander
Copy link
Copy Markdown
Contributor

This is killing inclusive our current number-of-active-users data, leaving us blind.

@rafaellehmkuhl I would note that usage information is not a development right or requirement - it's just helpful (and even then mostly in the trends). Consensual data collection means users can feel good about it (and not like we're being shady), and providing a "no ongoing telemetry" option is genuine while discouraging privacy-focused devs/users from making a fork that strips out the telemetry system entirely (which, by the license, they have the right to do).

This opt-out non-dismissible approach is going to hit us hard. I don't believe we should go that way.

I am curious what you're meaning here - opt-out is already encouraging users to allow the telemetry, right?

If your issue is with the "non-dismissible" part, we could potentially have an "ask me next time" / "ask me later" button that keeps the settings as undefined, with no telemetry sent that session? Not really sure what the alternatives would be 🤷‍♂️

@ES-Alexander ES-Alexander added docs-needed Change needs to be documented privacy labels Apr 24, 2026
@rafaellehmkuhl
Copy link
Copy Markdown
Member

rafaellehmkuhl commented Apr 24, 2026

This is killing inclusive our current number-of-active-users data, leaving us blind.

@rafaellehmkuhl I would note that usage information is not a development right or requirement - it's just helpful (and even then mostly in the trends). Consensual data collection means users can feel good about it (and not like we're being shady), and providing a "no ongoing telemetry" option is genuine while discouraging privacy-focused devs/users from making a fork that strips out the telemetry system entirely (which, by the license, they have the right to do).

We are not being shady. We can and should inform the users about the existence of telemetry. Opening an automatic window and asking/forcing the user to opt is just going to make people disable it unnecessarily.

Not only that, but we don't collect non-anonymous data, and we only collect non-intrusive events. We are on the other side of the fence right now, by being very limited on what we collect.

This opt-out non-dismissible approach is going to hit us hard. I don't believe we should go that way.

I am curious what you're meaning here - opt-out is already encouraging users to allow the telemetry, right?

If your issue is with the "non-dismissible" part, we could potentially have an "ask me next time" / "ask me later" button that keeps the settings as undefined, with no telemetry sent that session? Not really sure what the alternatives would be 🤷‍♂️

The alternative is to leave things as they are right now, add this dialog to be opened from the menu (as we have the full-opt-out-switch today and add more information in our README and docs.

If you look for any studies and AB tests you will find that the reduction in telemetry data from going from opt-out to opt-in vary between 99% to 70%. From our numbers that would mean receiving data about around 1-30 users, so basically negligible. It kills the entire purpose of having telemetry for being able to provide better solutions and define directions.

If any users have problems with that, I would say they are indeed encouraged to fork the project. It doesn't make sense to harm the work because of anonymous and hand-crafted telemetry.

@ES-Alexander
Copy link
Copy Markdown
Contributor

We are not being shady.

I wasn't suggesting that - just that if we substantially increase telemetry without informing users then that could be interpreted (by some users) as shady.

We can and should inform the users about the existence of telemetry.

I think we all agree on this 👍

Opening an automatic window and asking/forcing the user to opt is just going to make people disable it unnecessarily.

Opt-in vs opt-out is based on the defaults we specify (e.g. where does the slider start, in the case of this PR). Forcing some kind of decision is somewhat independent of that, though I suppose if we don't force a decision then that would be a stronger variant of opt-out (or opt-in, depending on the default).

I don't think anyone is suggesting opt-in as the default behaviour 👍

The alternative is to leave things as they are right now, add this dialog to be opened from the menu (as we have the full-opt-out-switch today and add more information in our README and docs.

If I'm understanding correctly, your main concern here is that opting out is too easy/likely when confronted with the current UX?

I think that's workable, but I do think we should prominently mention application telemetry in the application as part of informing users.1 If that seems reasonable, would it be sufficient if there are two dialogs instead? Something like:

  1. On startup, we just have a notice that there is more telemetry now, with an "Ok - I'm in" button (that turns on everything), and a separate "let me decide" / "give me options" button/link
    • The notice could mention (at a high level) the types of data being collected, and the purpose
    • For users that already have the toggle switched off we could avoid a notice entirely, since by default it won't affect them
      • We could also choose to prompt them anyway (possibly with an "Ok - still not interested" button), in case they decide to change their mind and provide some telemetry, if they didn't put much thought into it originally
  2. The settings menu (and the "let me decide" button) go to the existing (new in this PR) modal, with the slider and the breakdown of different options

Footnotes

  1. As an example, I like the GDPR's concept of active, informed consent. Note (in case it's relevant) that we likely aren't legally required to follow GDPR's interface suggestions here, because as far as I can tell the data currently being collected can't be reasonably linked/tied to any individual (but I'm also not a lawyer, or privacy expert 🤷‍♂️ )

@ArturoManzoli
Copy link
Copy Markdown
Contributor Author

The alternative is to leave things as they are right now, add this dialog to be opened from the menu (as we have the full-opt-out-switch today and add more information in our README and docs.

I understand your point, and I agree now that if we show the dialog on startup, we're making it too easy for people to opt out.
If we do need to inform the extended data collection, then we just show a snackbar once at startup (it can even be a persistent one that the user has to click X to close), saying that we have a new telemetry payload type and that it can be checked in Settings → General → Shared Data (also change the label from “Data Privacy” to "Shared Data").

@ES-Alexander
Copy link
Copy Markdown
Contributor

ES-Alexander commented Apr 27, 2026

If we do need to inform the extended data collection...

I think we do, but only for users who haven't already opted out (since they're not affected by the change).
The previously agreed-to setting now applies to more details about their systems and usage, which is their information to control the sharing of (regardless of whether we collect and use it in a way that is anonymised).

...we just show a snackbar once at startup (it can even be a persistent one that the user has to click X to close)

Fair enough. I'd encourage that persistence, just so there's definitely enough time to read it (lots can be happening during startup otherwise, including physical vehicle setup).

...change the label from “Data Privacy” to "Shared Data"

Agreed that "Data Privacy" seems like the wrong title. "Shared Data" is better, but perhaps ambiguous about why there is such a thing / what it would be for (maybe users would think it's public, or be concerned their actual operating data is being shared)? I think "App Telemetry" is maybe clearer, but hopefully we can come up with something better 🤷‍♂️

Likely out of scope for this PR, but I feel like this kind of configuration could even be rolled into a broader "Cockpit Improvements" page/interface, that lists out the release history and change log (including for available updates?), provides configuration for app telemetry, and adds some more direct means of generating user feedback suggestions and the like? Seems well-aligned with other things we've discussed doing, and positions the feature well with its actual intent :-)

@ArturoManzoli
Copy link
Copy Markdown
Contributor Author

ArturoManzoli commented Apr 27, 2026

@ES-Alexander
As you suggested earlier, making the 'Example Data' a real example may encourage the users to share, since it makes clear that we're not sending any sensitive information. Take a look:

image

@ArturoManzoli ArturoManzoli force-pushed the 2573-add-telemetry-agreement branch from 0df96ba to 67024d7 Compare April 27, 2026 14:47
@ArturoManzoli
Copy link
Copy Markdown
Contributor Author

@rafaellehmkuhl @ES-Alexander

I've pushed a new version, that is more inline with what we have been discussed here.
The PR description was also updated with new video and topics

Copy link
Copy Markdown
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

@rafaellehmkuhl @ES-Alexander

I've pushed a new version, that is more inline with what we have been discussed here. The PR description was also updated with new video and topics

I believe it improved a lot, but we need a dedicated discussion to settle.

Some things to change:

image .
  1. Right now it looks like the "descriptions" (e.g.: CPU/GPU type, available memory, ...) are an example, when they are actually exactly what is going to be sent. I believe we can improve on that by moving it to a row below the "On: ..." sentence with something like "Includes exactly: xxx". This way the users has a better chance of not wondering what else besides the examples is sent.
  2. "Locale" is a term that non-developers will mostly sure not know what it is and assume its their GPS position. We should be clear that it's the system language.
  3. MAVLink ID is not a thing. We should mention "Autopilot firmware type" instead.
  4. "Runtime" sounds like "everything that you do during the session". We should rename it to something like "Usage time".
  5. "Recording activity" is too generic and sounds like we have access to the recorded data. It should be clear what we are actually sending, like "Recording time".
  6. The tier system is not going to work. If the user doesn't want to share error data for example, they won't share vehicle type? Doesn't sound right and will force people even more to just select "no telemetry" so they don't have to think about it.
  7. The "Show example data" should be renamed to something like "Show exactly what is going to be shared". This gives more confidence to the user. "Example data" sounds like "it's going to be more or less like that".
  8. App version and running time should not be optional. We already use this data and it's the bare minimum for us to have an idea on how the user-base is growing (or not).
  9. When a user disables all or some telemetry, we should still send the minimum proposed and send in the package the telemetry choices of the user, so it becomes more clear how much are we losing.

I've made this list with what I consider to be the bare minimum changes, but honestly, my view is that this should not exist.

We should just not allow internal telemetry blocking and that's it. We are not sending any kind of personal information, all of those are completely inoffensive info and we don't sell user data nor anything like that.

If the user really wants to disable telemetry for some personal reason or view of the world, they should just use a traffic/ad-blocker.

In my personal view, this kind of telemetry policy is why many open source projects struggle to go in the right direction.

@ArturoManzoli ArturoManzoli force-pushed the 2573-add-telemetry-agreement branch from 67024d7 to ca5f57e Compare May 8, 2026 14:42
@ArturoManzoli ArturoManzoli changed the title Telemetry: clarify application telemetry and improve privacy controls Telemetry: Add application telemetry settings dialog May 8, 2026
Comment thread src/libs/external-telemetry/event-tracking.ts Outdated
Comment thread src/main.ts Outdated
Comment thread src/main.ts Outdated
@ArturoManzoli ArturoManzoli force-pushed the 2573-add-telemetry-agreement branch 3 times, most recently from 6b2b0fe to 35bd7ad Compare May 8, 2026 15:59
@ArturoManzoli ArturoManzoli force-pushed the 2573-add-telemetry-agreement branch from 35bd7ad to 8a20f96 Compare May 8, 2026 16:56
@ArturoManzoli ArturoManzoli force-pushed the 2573-add-telemetry-agreement branch from 8a20f96 to cedbf57 Compare May 8, 2026 17:01
Copy link
Copy Markdown
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

Tested and it's working.

@ArturoManzoli ArturoManzoli removed the request for review from ES-Alexander May 8, 2026 17:24
@ArturoManzoli ArturoManzoli dismissed ES-Alexander’s stale review May 8, 2026 17:25

Already reviewed

@ArturoManzoli ArturoManzoli merged commit b3f49e2 into bluerobotics:master May 8, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-needed Change needs to be documented privacy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants