fix: close structural telemetry gaps (issue #255)#259
Merged
Conversation
Rider had no consumer for telemetry/event at all, so every already-implemented event (discovery, go-to-step, rename, etc.) produced zero data for Rider users. Add ReqnrollTelemetryEventInterceptor + RiderTelemetryTransmitter, giving Rider parity with VS's TelemetryEventInterceptor and VS Code's telemetry.ts by forwarding to the same Application Insights resource. Separately, the LSP server was wired with NullTelemetryService for ITelemetryService, so MonitorError calls from LSP.Core parsing (DeveroomGherkinParser/DeveroomTagParser) were silently dropped in production. Add LspErrorTelemetryService, which forwards MonitorError to ILspTelemetryService as an Error event (redacting filesystem paths per the architecture doc's privacy requirement) while keeping every other host-UI-only member a no-op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merged
3 tasks
…anges Updates the aspirational telemetry design section to reflect what's actually implemented: Rider now has its own telemetry/event interceptor + transmitter (previously only VS and VS Code did), and the LSP server's ITelemetryService no longer no-ops MonitorError — LSP.Core exceptions now reach an Error telemetry/event with paths redacted, closing the privacy requirement already documented but previously unimplemented on the server side. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ility gaps ITelemetryService had ~16 members but only MonitorError was ever genuinely shared between LSP.Core and the VS host — every other member is a VS-lifecycle concern (project wizards, welcome/upgrade dialogs) that LSP.Core has no business depending on, forcing every LSP-side implementation (NullTelemetryService, LspErrorTelemetryService) to stub out ~15 unrelated no-ops just to satisfy the interface. - Add IErrorTelemetryService (MonitorError only); ITelemetryService now extends it. DeveroomGherkinParser, DeveroomTagParser, and CompletionContextResolver depend on the narrow interface directly. IIdeScope.TelemetryService stays typed as the full ITelemetryService, since VS's wizard flows genuinely need it — both interfaces resolve to the same DI singleton on the LSP server. - Fix a previously-unnoticed bug surfaced by this split: LspIdeScope.TelemetryService was hardcoded to NullTelemetryService instead of the DI-registered service, so errors reported through that access path (e.g. WatchedFilesHandler's config-load exceptions) were silently dropped even after the earlier MonitorError fix. - Wire MonitorOpenFeatureFile: implementation was real but never called; hooked into VsProjectEventMonitor's document-activation phase machine (fires once per open-lifetime). - Wire MonitorExtensionDaysOfUsage: WelcomeService already tracked status.UsageDays but never reported it. - Restore the "Extension loaded" signal (MonitorOpenProjectSystem) at ReqnrollPluginPackage's WelcomeService trigger point, without reintroducing the old wizard-trigger coupling. - Delete MonitorLoadProjectSystem: confirmed fully inert everywhere, including in its own "real" VS implementation. - Document (not wire) MonitorParserParse and MonitorReqnrollGeneration: the former is superseded by perf-sampling telemetry (VS no longer parses locally), the latter has no live feature to hook into (code-behind generation isn't implemented anywhere in this repo). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #255
Summary
telemetry/eventat all, so every already-implemented event (discovery, go-to-step, rename, comment-toggle, find-unused-steps, go-to-hook) produced zero telemetry data for Rider users. AddsReqnrollTelemetryEventInterceptor+RiderTelemetryTransmitter, giving Rider parity with VS'sTelemetryEventInterceptorand VS Code'stelemetry.tsby forwarding to the same Application Insights resource. ThetelemetryEventhook point was confirmed against JetBrains' public mirror at the exact pinned platform version (Rider 2024.3.5 / branch243), not guessed.NullTelemetryServiceforITelemetryService, soMonitorErrorcalls from LSP.Core parsing (DeveroomGherkinParser/DeveroomTagParser) were silently dropped in production. AddsLspErrorTelemetryService, which forwardsMonitorErrortoILspTelemetryServiceas anErrorevent (redacting filesystem paths from the message per the architecture doc's privacy requirement) while keeping every other host-UI-only member a no-op.Update: also folds in an Interface Segregation refactor of
ITelemetryService, prompted by review discussion on this PR. Only 1 of its ~16 members (MonitorError) was ever genuinely shared betweenLSP.Coreand the VS host — every other member is a VS-lifecycle concern (project wizards, welcome/upgrade dialogs) that forced every LSP-side implementation to stub out ~15 unrelated no-ops. This commit:IErrorTelemetryService(MonitorErroronly);ITelemetryServicenow extends it.DeveroomGherkinParser,DeveroomTagParser, andCompletionContextResolverdepend on the narrow interface directly.IIdeScope.TelemetryServicestays typed as the fullITelemetryService(VS's wizard flows genuinely need it) — both interfaces resolve to the same DI singleton on the LSP server.LspIdeScope.TelemetryServicewas hardcoded toNullTelemetryServiceinstead of the DI-registered service, so errors reported through that path (e.g.WatchedFilesHandler's config-load exceptions) were still being dropped even after theMonitorErrorfix above.MonitorOpenFeatureFile(implementation was real, never called — hooked intoVsProjectEventMonitor's document-activation phase machine) andMonitorExtensionDaysOfUsage(WelcomeServicetracked the count but never reported it); restores the "Extension loaded" signal (MonitorOpenProjectSystem) at its new trigger point; deletesMonitorLoadProjectSystem(confirmed fully inert everywhere).MonitorParserParse/MonitorReqnrollGenerationare documented rather than wired — the former is superseded by perf-sampling telemetry, the latter has no live feature to hook into.Structural gaps only — usage-event additions are #260, also part of #255. Not using a closing keyword here since #255 isn't fully addressed until both PRs land.
Test plan
dotnet build Reqnroll.IdeSupport.slnx— clean, 0 warnings/errorsdotnet test tests/LSP/Reqnroll.IdeSupport.LSP.Server.Tests— 622/622 passingdotnet test tests/VisualStudio/Reqnroll.VisualStudio.Tests— 118/118 passingdotnet test tests/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Wizards.Tests— 65/65 passing (2 new tests forMonitorExtensionDaysOfUsage)dotnet test tests/LSP/Reqnroll.IdeSupport.LSP.Server.Specs— 144/144 passing (18 pre-existing skips, unrelated)ReqnrollTelemetryEventInterceptor/RiderTelemetryTransmitter) could not be built/tested in this environment — the sandbox's JVM can't complete the Gradle wrapper's TLS handshake to download Gradle 8.10 (unrelated to the code change; reviewed manually against the real platform API, but needs a./gradlew build/runIdepass before merge)MonitorOpenFeatureFile's new wiring inVsProjectEventMonitor(DTE/COM-heavy) has no automated coverage — the existing test file for this class only covers theIVsTrackProjectDocumentsEvents2file-tracking half, not theWindowActivatedflow; the addition is defensively try/caught so it can't break document activation, but hasn't been verified live in VS🤖 Generated with Claude Code