feat(telemetry): report which host event and which params are triggered (SCAL-333657) - #634
feat(telemetry): report which host event and which params are triggered (SCAL-333657)#634sastaachar wants to merge 3 commits into
Conversation
SCAL-333657 trigger() uploaded `visual-sdk-trigger-<HostEvent>` with no properties, so the only answerable question was whether a host event fired at all, across ~90 separate Mixpanel event names. Which parameters customers actually use, which embed component triggered, and how the trigger resolved were all invisible. Adds a `visual-sdk-host-event` upload, fired once when a trigger settles or bails out, with `hostEvent` as a property so one report can rank host events and their parameters. The existing per-event upload keeps its name for existing dashboards and now carries the same properties. Payload values never leave the browser: a value is reported as its `typeof` (`name:string`), booleans included. SDK enum members are the one exception (`operator:EQ`) - a fixed token from our own contract - and only when the key is a known enum-valued parameter and the value matches that enum exactly. Key names are reported only when they read as code identifiers, since a payload can be keyed by a customer column name, and error messages are never uploaded - only a status and our own EmbedErrorCodes. Two further fixes fall out of this: - A trigger the embedded app never answered was indistinguishable from a successful one, because processTrigger resolves, rather than rejects, with Error(TRIGGER_TIMED_OUT). It now reports status 'timed-out'. - The pre-init event queue in mixpanel-service was unbounded, and with `disableSDKTracking` set initMixpanel never runs, so the queue grew for the lifetime of the page. Capped at 100. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive telemetry for host events, allowing the SDK to report which host events are triggered, their parameter shapes (with customer data redacted), and how they resolve. Key changes include adding a new visual-sdk-host-event Mixpanel event, implementing payload shape-walking utilities with safety limits, capping the pre-initialization Mixpanel event queue to prevent memory leaks, and adding extensive unit tests. The review feedback highlights two areas for improvement: updating a comment in the test file to use the current "liveboards" terminology instead of "dashboards" to comply with the style guide, and correcting a JSDoc comment in hostEventTelemetry.ts that incorrectly states boolean values are reported as-is when they are actually reported by their type.
|
|
||
| await embed.trigger(HostEvent.DownloadAsCsv, { vizId: 'd0a1' }); | ||
|
|
||
| // The per-event upload is kept for the existing dashboards, and now |
There was a problem hiding this comment.
❌ FOUND: "dashboards" in line 65 comment.
FLAG FOR UPDATE: Replace "dashboards" with "liveboards" to adhere to the current ThoughtSpot terminology.
Reference: Repository Style Guide line 146.
| // The per-event upload is kept for the existing dashboards, and now | |
| // The per-event upload is kept for the existing liveboards, and now |
References
- Replace 'Dashboard' or 'Pinboard' with 'Liveboard' as per the deprecated terminology mapping. (link)
There was a problem hiding this comment.
Not applied — the term here refers to Mixpanel dashboards, not ThoughtSpot Liveboards, so the substitution would have made the comment false.
Reworded instead, in 64dcca43, to avoid the ambiguous term without asserting something untrue:
// The per-event upload keeps its name so existing Mixpanel reports
// still work, and now carries the same properties.
| * `runtimeFilters[].columnName:string`, `start:true`. Boolean values are | ||
| * reported as-is because the value is the usage signal and carries no | ||
| * customer data; every other value is reduced to its type. |
There was a problem hiding this comment.
There is a discrepancy between the JSDoc description and the actual implementation/test suite. The JSDoc states that boolean values are reported as-is (e.g., start:true), but both the implementation in describeLeaf and the test suite (reports a boolean by its type, not its value) reduce booleans to their type ('boolean').
Please update the JSDoc to align with the actual behavior.
| * `runtimeFilters[].columnName:string`, `start:true`. Boolean values are | |
| * reported as-is because the value is the usage signal and carries no | |
| * customer data; every other value is reduced to its type. | |
| * 'runtimeFilters[].columnName:string', 'start:boolean'. Boolean values are | |
| * reported by their type; every other value is also reduced to its type. |
commit: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return this.hostEventClient | ||
| .triggerHostEvent(messageType, data, context, (dispatchRoute) => { | ||
| route = dispatchRoute; | ||
| }) | ||
| .then((response) => { | ||
| // processTrigger resolves — it does not reject — with an Error | ||
| // when the embedded app never answers, so a timed-out trigger | ||
| // is otherwise invisible. | ||
| const settled = response as unknown; | ||
| reportHostEvent( | ||
| settled instanceof Error | ||
| && settled.message === ERROR_MESSAGE.TRIGGER_TIMED_OUT | ||
| ? 'timed-out' | ||
| : 'success', | ||
| ); | ||
| return response; | ||
| }) |
There was a problem hiding this comment.
Correctness: for host events dispatched through the custom-handler branch — Pin, SaveAnswer, UpdateFilters, DrillDown — a real timeout is not reported as status: 'timed-out' here; it comes through as a generic status: 'error' with no errorCode.
Trace: processTrigger resolves (does not reject) with new Error(ERROR_MESSAGE.TRIGGER_TIMED_OUT) on timeout (src/utils/processTrigger.ts:77). For these four events, triggerHostEvent dispatches through customHandler, which for all of them eventually calls handleHostEventWithParam (host-event-client.ts:90-102):
const response = (await this.triggerUIPassthroughApi(apiName, parameters, context))?.find?.(...)
if (!response) { throw { error: `No answer found...` }; }An Error instance has no .find, so response becomes undefined and the function throws a plain { error } object instead of letting the resolved Error propagate. That plain object reaches this file's .catch() with isValidationError undefined, so it falls into the else branch at line 1784 and reports status: 'error' — the settled instanceof Error check in the .then() above never runs because the promise rejected instead of resolving.
Net effect: a real 30s timeout on Pin/SaveAnswer/UpdateFilters/DrillDown is indistinguishable in the new telemetry from any other error, which undercounts exactly the signal ('timed-out') this PR was built to surface for those four events.
| embedComponentType: this.viewConfig?.embedComponentType, | ||
| }); | ||
| const triggerStartedAt = Date.now(); | ||
| let route: HostEventRoute; |
There was a problem hiding this comment.
Minor type-accuracy nit: route is typed as HostEventRoute (never undefined), but it stays unset whenever reportHostEvent fires before hostEventClient.triggerHostEvent is ever called — the render-not-called (line 1711), host-event-undefined (line 1722), and no-iframe (line 1738) branches. It works today only because ...(route ? { route } : {}) treats the uninitialized value as falsy; the declared type doesn't reflect that possibility. Consider let route: HostEventRoute | undefined; so the type matches actual runtime states.
| }), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage: every test here drives HostEvent.DownloadAsCsv, which always resolves to route: 'legacy'. Nothing in this file (or hostEventTelemetry.spec.ts) exercises the 'custom-handler' or 'ui-passthrough' values of HostEventRoute, even though onRoute in host-event-client.ts is new code introduced by this PR with three distinct branches. Worth a case that triggers e.g. HostEvent.Pin or a getter event and asserts route on the reported props.
Review verdict: no blocking issues foundReviewed the host-event telemetry addition ( Traced the main risk areas closely and didn't find a reproducible bug:
One thing worth a maintainer's own judgment call, not raised as a finding since it doesn't reproduce today: Test coverage is thorough — Style guideNo documentation findings — no |
Browser verification showed a passthrough getter reporting `route: 'ui-passthrough'` while the message was ultimately carried by the legacy channel, because getDataWithPassthroughFallback falls back when the app returns no usable response. The caveat was documented for custom handlers only; it applies to the getter branch as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verified in a real browser, not just jest — 32/32, twiceBuilt a credential-free harness for this: the SDK is bundled from source, What it confirms live, beyond what the unit tests pin:
And the assertion that matters most: no customer value appears in any of the 26 host event uploads across a full run, scanned for It also confirmed the pre-existing leak empiricallyThe same run observes One doc fix pushed as a result (
|
| /** | ||
| * Key paths annotated with value type — `runtimeFilters:array(3)`, | ||
| * `runtimeFilters[].columnName:string`, `start:true`. Boolean values are | ||
| * reported as-is because the value is the usage signal and carries no | ||
| * customer data; every other value is reduced to its type. | ||
| */ |
There was a problem hiding this comment.
The doc comment contradicts the implementation and the test suite. describeLeaf reduces every non-null, non-enum leaf value (including booleans) to typeof value, so a boolean is reported as isPublic:boolean, never as start:true. This is directly asserted by the test 'reports a boolean by its type, not its value' in src/utils/hostEventTelemetry.spec.ts, which expects ['isPublic:boolean', 'runRuntimeFilters:boolean']. As written, this comment tells a future reader raw boolean values are exposed, which is the opposite of what happens and of the privacy invariant the rest of the file is built around.
| /** | |
| * Key paths annotated with value type — `runtimeFilters:array(3)`, | |
| * `runtimeFilters[].columnName:string`, `start:true`. Boolean values are | |
| * reported as-is because the value is the usage signal and carries no | |
| * customer data; every other value is reduced to its type. | |
| */ | |
| /** | |
| * Key paths annotated with value type — `runtimeFilters:array(3)`, | |
| * `runtimeFilters[].columnName:string`, `isPublic:boolean`. Every value | |
| * is reduced to its type, except an SDK enum member (see the module | |
| * comment), so no customer value ever appears here. | |
| */ |
SCAL-333657
Review catch. For the four host events dispatched through a custom handler —
Pin, SaveAnswer, UpdateFilters, DrillDown — a real 30s timeout was reported
as a generic `status: 'error'`, which undercounted exactly the signal this
work exists to surface.
processTrigger resolves, rather than rejects, with Error(TRIGGER_TIMED_OUT).
An Error has no `.find`, so handleHostEventWithParam saw a missing response
and threw a plain `{ error: 'No answer found' }`, losing the timeout before
trigger() could classify it. The `settled instanceof Error` check in the
success path never ran, because the promise had rejected.
The thrown shape is unchanged, so nothing a host application catches today
moves; an `isTimeout` flag rides alongside it and trigger() reports
'timed-out'. The timeout predicate now lives once, in processTrigger, and
both call sites share it.
Also from review:
- The paramShape doc comment still claimed booleans are reported as-is; they
have been reduced to `boolean` since the typeof-not-value rule landed.
- `route` is declared `HostEventRoute | undefined`, matching the branches that
report before dispatch ever happens.
- Tests for the 'custom-handler' and 'ui-passthrough' routes, the legacy
fallback, and a regression test for the timeout above — verified to fail
without the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review addressed in 1. Timed-out custom-handler setters reported as generic errors — fixed. The trace was exactly right: Fixed without moving anything a host application catches today: the thrown shape is unchanged, an Pinned by a regression test, and I confirmed it fails without the fix — 2. Missing route coverage — added. Four tests: 3. 4. 5. On the "dashboards" → "liveboards" suggestion — not applied, deliberately. That comment is about Mixpanel dashboards, not ThoughtSpot Liveboards, so the substitution would have made it false. Reworded to "existing Mixpanel reports" instead, which removes the ambiguous term without asserting something untrue. Re-verified after the changes: full suite 1721 passed (47 files), and the browser harness still 32/32 including the real 30s timeout. |
|
Closed by a branch rename to match the The review round here is already addressed in |
1 participant
SCAL-333657 · epic SCAL-325536 (TSE SWAT: Q1 2027)
Why
trigger()uploadedvisual-sdk-trigger-<HostEvent>with no properties, so the only answerable question was whether a host event fired at all — across ~90 separate Mixpanel event names. Invisible today: which parameters of a host event customers actually use, which embed component triggered it, whether the trigger succeeded / failed / timed out, how long it took, and whether UI passthrough or the legacy postMessage channel served it.What it emits
Two uploads per trigger:
visual-sdk-trigger-<HostEvent>visual-sdk-host-event(new)hostEventis a property, so one report ranks host events and their parameters instead of needing one report per event nameProperties:
hostEvent,embedComponentType,contextType,sdkVersion,hasPayload,payloadType,paramCount,paramKeys,paramShape,shapeTruncated,status,durationMs,route,errorCode.Payload values never leave the browser
Host event payloads carry customer data — GUIDs, filter values, search strings, column names. So a value is reported as its
typeof:name:string, nevername:"Quarterly revenue". Booleans included.SDK enum members are the one exception —
operator:EQis a fixed, low-cardinality token from our own contract, and knowing which operator customers pass is the point of the exercise. A value is treated as an enum member only when its key is a known enum-valued parameter and the value matches that enum exactly, so a free-form string under the same key still degrades tostring. Mapped today:operator/oper→RuntimeFilterOp,level→ApplicabilityLevel.Two further guards: key names are reported only when they read as code identifiers (a payload can be keyed by a customer column name, so
"Total Sales"becomesredactedKey), and error messages are never uploaded — onlystatusplus our ownEmbedErrorCodes.Opt-out is inherited, not new:
EmbedConfig.disableSDKTrackingalready stopsinitMixpanel.Two fixes that fall out of this
processTriggerresolves — it does not reject — withError(TRIGGER_TIMED_OUT)after 30 s, so a host event the embedded app never answered looked identical to a success. Now reported asstatus: 'timed-out'.uploadMixpanelEventqueues untilinitMixpanelruns, and withdisableSDKTracking: trueinitMixpanelis never called — so the queue grew for the lifetime of the page. Capped atMAX_QUEUED_EVENTS = 100; flush semantics unchanged.Public API
MIXPANEL_EVENTis re-exported fromindex.ts, so the newVISUAL_SDK_HOST_EVENTkey is an additive public change. The enum is@hidden, so it is not in the published docs and needs no@versiontag.HostEventClientis not exported publicly, so its new optionalonRouteargument is internal only. No existing name, value or default changes.React needs no change:
useEmbedRef().current.trigger(...)calls straight through toTsEmbed.trigger(), so React consumers are covered by the same path.Not in this PR — please read
VISUAL_SDK_EMBED_CREATE(src/embed/ts-embed.ts) doesuploadMixpanelEvent(..., { ...viewConfig })— it spreads the entire viewConfig into Mixpanel, includingliveboardId,runtimeFilterswith values andsearchOptionswith the search string. That is customer data reaching Mixpanel today, on every embed construction. Deliberately left alone here to keep this PR reviewable; it needs its own ticket, and the shape-not-values helper added here is what fixes it.Testing
src/utils/hostEventTelemetry.spec.ts(19) — type-not-value reporting, enum members by both spellings, enum fallback when the value is not a member, key redaction, depth and path caps, cyclic and throwing-getter payloads, and a direct "never reports a payload value" assertion.src/embed/host-event-telemetry.spec.ts(5) — end-to-end through a renderedLiveboardEmbed: success plus route, parameter names and enum members with customer values asserted absent, timed-out, error without its message, trigger-before-render.src/mixpanel-service.spec.ts— queue cap.Full suite 47 files / 1717 passed,
tscclean,lint0 errors and no new warnings,check-size31.71 kB against the 34 kB budget.Open question for review
The property names become a Mixpanel contract the moment reports are saved against them — a rename afterwards invalidates those reports. Utsav — worth a look at the property list above before this merges.
🤖 Generated with Claude Code