feat(plugin-ext): gate plugin telemetry on the telemetry consent provider - #17963
feat(plugin-ext): gate plugin telemetry on the telemetry consent provider#17963dr14-make wants to merge 1 commit into
Conversation
51b77ba to
1a864bf
Compare
|
I have verified this end to end, including against a real telemetry backend, so I can confirm the gate works rather than only that the tests pass. In
Changing the preference while the application runs also works: I then tried it in a downstream Theia application that bundles an extension routing all of its analytics through That downstream case is the one I care most about: the extension builds its logger and logs its first event on consecutive statements of Worth flagging for whoever reviews the |
…ider `@theia/telemetry` shipped `TelemetryConsentProvider` and the `telemetry.telemetryLevel` preference, but nothing connected them to the plugin host. `TelemetryExtImpl` had telemetry hardcoded off, so `env.isTelemetryEnabled` was always false and every `TelemetryLogger.logUsage` and `logError` returned early. Applications collected nothing from plugins and users could not opt in. The plugin host now carries a `TelemetryLevel` instead of a boolean. A boolean cannot express level `error`, where errors are collected but usage is not, so carrying one made that level impossible to represent. Levels are compared with `isKindAllowedByLevel` so the ordering stays in one place. Plugins create their loggers while they activate, so a level that only arrived over RPC would drop everything logged during activation. The level is passed in `PluginManagerInitializeParams` and applied in `$init` before any plugin activates; RPC then only carries later changes. To apply it there, the plugin manager needs the telemetry ext, so `TelemetryExtImpl` is bound in both plugin host containers and passed to `createAPIFactory` like `EnvExtImpl` is. The frontend reads the level from `TelemetryConsentProvider` rather than from `params.preferences`, because the provider is meant to be rebindable. Contributes to eclipse-theia#17847 Signed-off-by: Dmitrij Rozdestvensky <dmitrij.rozdestvensky@juliahub.com>
1a864bf to
29e32c5
Compare
What it does
Contributes to #17847.
@theia/telemetry(#17809) addedTelemetryConsentProviderand thetelemetry.telemetryLevelpreference, but nothing connects them to the plugin host.TelemetryExtImplhas telemetry hardcoded off, soenv.isTelemetryEnabledis alwaysfalseand everyTelemetryLogger.logUsageandlogErrorreturns early. Applications collect nothing from plugins, and users have no way to opt in.This connects the two. The resulting states match VS Code (
extHostTelemetry.ts):telemetry.telemetryLevelenv.isTelemetryEnabledlogger.isUsageEnabledlogger.isErrorsEnabledoffcrasherrorallNote that
env.isTelemetryEnabledislevel === 'all', notlevel !== 'off'.Three points worth a look during review.
The plugin host now carries a
TelemetryLevelinstead of a boolean. A boolean cannot express levelerror, where errors are collected but usage is not. This replaces the_telemetryEnabledfield and theupdateEnableStatescollapse added in #17693, and rewrites that PR's spec, which tested the boolean behaviour. Levels are compared withisKindAllowedByLevelfrom@theia/telemetryso thecrash/error/allordering stays in one place.logUsageandlogErrorcheck both the level and the per-kind flag, so a plugin that setsisUsageEnabledon its own logger still cannot send anything the user did not consent to.The first level is passed in
$init, not pushed over RPC. Plugins create their loggers while they activate, andcreateAPIFactoryruns before$init, so a level that only arrived over RPC would drop everything logged during activation.PluginManagerInitializeParamsgets an optionaltelemetryLevel(absent meansoff, which is what hosts that do not set it get today), applied in$initnext to the existingenvExt.set*calls. RPC then only carries later changes. To apply it there the plugin manager needs the telemetry ext, soTelemetryExtImplis bound in both plugin host containers and passed tocreateAPIFactory, the same wayEnvExtImplis.The frontend reads the level from
TelemetryConsentProvider, not fromparams.preferences, because the provider is meant to be rebindable.That means
@theia/plugin-extnow depends on@theia/telemetry. All four example apps already pull it in, so it resolves today, but plugin-ext does need the binding now, and an application that ships plugin-ext without telemetry would fail incontainer.get. If the dependency is a problem, the alternative is anisBoundguard that defaults tooff. Happy to switch.TelemetryLoggeralso takes the level-changeEventand owns the subscription, so a disposed logger no longer stays referenced by the emitter for the lifetime of the plugin host. That leak already existed, but the constructor changes here anyway, so fixing it now avoids a second breaking change later.Also fixes a stray paren in the
TELEMETRY_EXTproxy identifier string ('TelemetryExt)'). It is only used as a key, so it does no harm today.Two things that are not visible in the diff:
$initis current only becausestartPluginsawaitstheiaReadyPromise, which includespreferenceServiceImpl.ready, the same promisePreferenceTelemetryConsentProvideruses to set its level. That spans three files andcreateTheiaReadyPromiseisprotected, so an override that dropspreferenceServiceImpl.readywould bring back the dropped activation events. A reboundTelemetryConsentProviderthat reads consent from a file or over the network has no such barrier either and would reportoffat that point. If that is a case we want to support,TelemetryConsentProviderprobably needs a readiness promise. I am happy to follow up separately.package-lock.jsonhas one hand-added line for the new workspace dependency. The regeneration script indoc/lockfile-maintenance.mdneeds Node 24, and regenerating on an older npm stripslibcfields and platform-specific optional entries. For a workspace-internal dependency with no install scripts and no platform binaries the result is the same, andverify-lockfile-platforms.jspasses locally. Let me know if you would rather have it regenerated.The new API is marked
@experimental, like everything added in #17809.This change was generated by AI, then validated and reviewed by a human. I have read, reviewed and tested all of it myself, and I am accountable for it as the author.
How to test
npx lerna run test --scope @theia/plugin-ext.telemetry-ext.spec.tscovers every row of the table above, the state transitions, logger disposal, and a logger created while the level forbids usage.telemetry-main.spec.tscovers pushing level changes to the plugin host.By hand, with a plugin that calls
logUsageandlogErrorfrom itsactivate():"telemetry.telemetryLevel": "all"and start the application. The events logged during activation reach the sender, andenv.isTelemetryEnabledistrueinsideactivate().off, the default. Nothing reaches the sender.error.logUsageis dropped,logErroris delivered.onDidChangeEnableStatesfires and the logger flags follow.env.onDidChangeTelemetryEnabledonly fires when the value it reports changes, socrashtoerrordoes not fire it.I ran all four steps in
examples/browseragainst both plugin hosts: the Node host with a VS Code plugin, and the web worker host with a Theia frontend plugin.Follow-ups
The rest of #17847: forwarding plugin telemetry into Theia's own topics and sinks. Also out of scope here:
TelemetryLogger.getCommonProperties()returns[], so there is nocommon.*injection.env.machineIdandenv.sessionIdare new UUIDs on every host start, somachineIdis not stable across restarts.Breaking changes
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers