SCAL-333657: track which host event is triggered and with which parameters

[sastaachar]

SCAL-333657 · epic SCAL-325536 (TSE SWAT: Q1 2027)

Replaces #634, closed by a branch rename. PR 1 of a split — see the plan at the bottom.

Which host event is triggered, and which parameters it uses. Nothing else.

Why

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 pass was invisible.

What changes

One existing line, into ten:

-uploadMixpanelEvent(`${MIXPANEL_EVENT.VISUAL_SDK_TRIGGER}-${messageType}`);
+uploadMixpanelEvent(
+    `${MIXPANEL_EVENT.VISUAL_SDK_TRIGGER}-${messageType}`,
+    getHostEventTelemetryProps({ hostEvent: messageType, payload: data, context, embedComponentType }),
+);

No existing SDK behaviour changes. Same event name, same timing, same call site, no new Mixpanel events, no signature changes, nothing removed. The event simply carries properties where before it carried none, so existing reports keep working and gain fields.

Properties: hostEvent, embedComponentType, contextType, sdkVersion, hasPayload, payloadType, paramCount, paramKeys, paramShape, shapeTruncated.

Payload values never leave the browser

Host event payloads carry customer data — GUIDs, filter values, search strings, column names. A value is reported as its typeof: name:string, never name:"Quarterly revenue". Booleans included.

SDK enum members are the one exception — operator:EQ is a fixed, low-cardinality token from our own contract, and knowing which operator customers pass is the point. A value is kept only when its key is a known enum parameter and it matches that enum exactly, so a free-form string under the same key still degrades to string.

Key names are reported only when they read as code identifiers, since a payload can be keyed by a customer column name — "Total Sales" becomes redactedKey.

{ runtimeFilters: [{ columnName: 'Region', operator: EQ, values: ['west'] }] }

paramKeys:  ['runtimeFilters']
paramShape: ['runtimeFilters:array(1)',
             'runtimeFilters[]:object(3)',
             'runtimeFilters[].columnName:string',   <- name kept, value never
             'runtimeFilters[].operator:EQ',         <- SDK enum, member kept
             'runtimeFilters[].values:array(1)']

Bounded: depth 3, 40 key paths. Cyclic payloads and throwing getters are handled — a hostile payload can never break the trigger it describes.

Opt-out is inherited, not new: EmbedConfig.disableSDKTracking already stops initMixpanel.

One known gap, left as a TODO in the code: the enum map is hand-maintained, so a host event that gains an enum parameter nobody adds will silently report string. A generated map, or reading members off the contract types, would fix it properly.

Public API

Nothing exported changes. The new module is internal — not re-exported from index.ts. No new MIXPANEL_EVENT keys, no changed signatures, no @version tags needed.

React needs no change: useEmbedRef().current.trigger(...) calls straight through to TsEmbed.trigger().

Testing

src/utils/hostEventTelemetry.spec.ts (19) — type-not-value reporting, enum members by both spellings, enum fallback for a non-member, key redaction, depth and path caps, empty containers and nulls, top-level array payloads, cyclic and throwing-getter payloads, and a direct "never reports a payload value" assertion.

src/embed/host-event-telemetry.spec.ts (3) — end-to-end through a rendered LiveboardEmbed: the properties land, the event name is unchanged, and customer values are absent while enum members survive.

Full suite 1714 passed (47 files), tsc clean, lint 0 errors, check-size within the 34 kB budget.

The rest of the split

Everything below was built and verified on this branch, then removed to keep PR 1 reviewable. It is preserved at bb33dd47 and will come back as its own PR:

• PR 2 — outcome and duration of a trigger. status (success / error / timed-out / render-not-called / no-iframe), durationMs, and the dispatch route (custom handler vs UI passthrough vs legacy postMessage).
• PR 3 — the response story, both directions. A trigger resolves with the app's answer; an embed event hands the host a responder (on(ApiIntercept, responder)). Whether it was answered, how long it took, and the shape of the answer.
• PR 4 — embed event tracking. Which embed events actually arrive, their payload shape, and how many handlers each dispatch ran.
• PR 5 — bug fixes found along the way, each independently reviewable: a 30s trigger timeout is currently indistinguishable from success because processTrigger resolves rather than rejects with its timeout Error; the same timeout is lost entirely for Pin/SaveAnswer/UpdateFilters/DrillDown; and the pre-init Mixpanel queue is unbounded when disableSDKTracking is set, so it grows for the lifetime of the page.

Separately, and not part of any of these: VISUAL_SDK_EMBED_CREATE uploads the entire viewConfig to Mixpanel — liveboardId, runtimeFilters with values, searchOptions with the search string. That is customer data going out today, on every embed construction. It needs its own ticket.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces comprehensive telemetry for host events, capturing event triggers, parameter shapes (with customer data redacted), routing paths, and resolution statuses (success, error, timeout) to Mixpanel. It also caps the pre-initialization Mixpanel event queue to prevent memory leaks when tracking is disabled. Feedback on the changes suggests using optional chaining on the parameters object in host-event-client.ts to prevent potential runtime crashes if it is nullish.

[gemini-code-assist]

To prevent potential runtime TypeError crashes if parameters is ever nullish (e.g., null or undefined), use optional chaining (parameters?.vizId) when checking for the presence of vizId.

Suggested change

	const error = `No answer found${parameters.vizId ? ` for vizId: ${parameters.vizId}` : ''}.`;
	const error = 'No answer found' + (parameters?.vizId ? ' for vizId: ' + parameters.vizId : '') + '.';

[github-actions]

Review summary

Reviewed the new host-event / embed-event telemetry pipeline (eventTelemetry.ts, mixpanel-service.ts, ts-embed.ts, host-event-client.ts) plus the accompanying tests. Overall this is well-scoped and defensively written — payload shape reporting is capped in depth and path count, keys are sanitized, values are never serialized (only typeof, with an explicit allowlist for enum members), the pre-init mixpanel queue is now bounded, and the timeout-vs-error signal is threaded consistently between processTrigger's resolve-with-Error convention and the thrown-object convention in handleHostEventWithParam.

One correctness finding, posted inline on src/embed/ts-embed.ts: in executeCallbacks, handlerCount can undercount when an earlier handler in the dispatch loop calls its responder synchronously (the documented pattern for ApiIntercept/OnBeforeGetVizDataIntercept) — the report fires and latches reported = true before later handlers in the same forEach run, so the final count doesn't include them. This directly undermines the stated goal of a prior fix in this stack ("count only the handlers a dispatch ran"), and isn't covered by the existing tests (which only exercise a single handler responding, or multiple handlers where none respond).

No breaking changes to the public API surface — HostEventClient and eventTelemetry.ts aren't re-exported from index.ts, and the new onRoute parameter on triggerHostEvent is optional and appended last. No version-tag or deprecated-terminology lines were touched in this diff.

Style guide

No documentation findings in this diff — no @version tags, deprecated terminology, or malformed doc comments were added or edited in the reviewed files.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@thoughtspot/visual-embed-sdk@635

commit: e4f27f3

[sastaachar]

Reshaped per feedback, in c42ddb99. Taking the points in order.

2. Send the extra data on the existing event — done, and it was the better call. The separate aggregate event is gone. visual-sdk-trigger-<HostEvent> keeps its name but now fires once, when the trigger settles, carrying the parameters, outcome and duration. One upload per trigger instead of two.

3. Duration, and a no-response event. Every upload now carries durationMs. When the embedded app never answers, visual-sdk-host-event-no-response goes out alongside the per-event upload — measured at 30001 ms in the browser harness.

4. Embed events tracked. executeCallbacks is the one point every embed event passes through, so a single hook covers all of them. New visual-sdk-embed-event reports the event name, payload shape, the event's own status, and handlerCount — which also answers "is anyone actually listening to this event", useful on its own.

5. Types only, no values, for embed payloads too. Same shape summariser, with a tighter cap (20 paths vs 40) because embed payloads are much larger. A Data event carrying { columnNames: ['Region'], rows: [['west', 100]], answerName: 'Quarterly revenue rollup' } reports:

data:object(3)  data.answerName:string  data.columnNames:array(1)
data.columnNames[]:string  data.rows:array(1)  data.rows[]:array(2)

6. Non-blocking, minimal cost. Three things: nothing is walked or built when a host application sets disableSDKTracking — it returns before touching the payload; telemetry is reported after the host's own handlers have run; and the walk itself is deferred to requestIdleCallback (2s timeout, setTimeout fallback). So an embed event on a hot path costs the caller one closure allocation. Two tests pin it: a disabled host uploads nothing at all, and the host handler is provably ordered before the telemetry.

1. Comments removed. All of them, from the new code — including the JSDoc stubs. Lint stays at 0 errors, so nothing required them.

7. The enum map — kept, with the note you asked for. It is the one comment left in the file, and it says what is wrong with it: the map is hand-maintained, an enum parameter nobody adds silently degrades to string, and nothing fails to tell you. It suggests generating the map or reading the members back off the contract types instead. Left as a TODO rather than pretending it is solved.

Verification

Full suite 1729 passed (47 files), tsc clean, lint 0 errors, check-size 31.71 kB against the 34 kB budget.

Browser harness 37/37, now including embed events driven over the real postMessage path rather than a direct call — so the whole chain (postMessage → processData → executeCallbacks → telemetry) is covered. No customer value in any of the 38 host + embed event uploads in a full run.

One incidental finding: render() itself dispatches an init embed event, so that shows up in the telemetry too. Correct behaviour and useful signal, but worth knowing before reading a report — the first embed event of any session is the SDK's own.

[github-actions]

handlerCount counts every handler registered for eventType (plus EmbedEvent.ALL), not the ones that actually ran for this dispatch. The forEach above only invokes callbackObj.callback when callbackObj.options.start matches dataStatus (line 1441-1445) — e.g. a caller that registers one { start: true } handler and one default (end) handler for the same EmbedEvent will see handlerCount: 2 reported on every single dispatch, even though only one of the two handlers ran for that particular start/end payload.

Since MessageOptions.start is a documented, supported way to subscribe separately to start/end (src/types.ts MessageOptions), this isn't a hypothetical: any embed that splits start/end handling for the same event type will get telemetry that overstates "whether anyone listens" for that specific payload.

Suggest counting only the handlers that were actually invoked, e.g. track a counter inside the forEach instead of using callbacks.length.

[github-actions]

reportEvent's buildProps closure (in src/utils/eventTelemetry.ts) is only evaluated later, inside a requestIdleCallback/setTimeout deferral — by design, so telemetry never blocks the host application's handler (covered by the "does not block the host application handler" test). But data is captured by reference: callbackObj.callback(data, responder) on line 1442 hands the same object to every registered handler before this deferred read happens. If any handler mutates data in place (e.g. normalizing/redacting a field on the object it was given), describePayload(data) will describe the post-mutation shape, not what was actually dispatched to on() listeners at line 1442. Not exploitable, but worth a code comment or a shallow-clone-before-reporting if the shape is meant to reflect what handlers received.

[sastaachar]

Registration noise cut, in d3e1ee15. You were right that it is mostly noise, and it turned out to be worse than that — the mechanism meant to suppress it was broken.

Measured, for a full-height Liveboard with two host handlers:

before:  12 uploads   appInit, ThoughtspotAuthExpired, IdleSessionTimeout,
                      EmbedListenerReady, authInit, RefreshAuthToken,
                      ROUTE_CHANGE, EMBED_HEIGHT, EmbedIframeCenter,
                      requestVisibleEmbedCoordinates,   <- 10 SDK internals
                      data, Error                       <- 2 host handlers
after:    2 uploads   data, Error

isRegisteredBySDK exists precisely to mark those internals, and it was not working:

1. V1Embed.on dropped it. The override declared three parameters and called super.on(eventType, callback, options) — so on any Liveboard, Pinboard or App embed, all six base-class internal registrations arrived marked isRegisteredBySDK: false, indistinguishable from a host registration. That is a pre-existing bug, not something this PR introduced; it just happens to be the thing that defeats the filter.
2. The full-height handlers never set it — four sites in liveboard.ts / app.ts called this.on(...) with two arguments.

Both fixed, and the on() upload now goes through reportEvent, so it respects disableSDKTracking and stays off the critical path like everything else.

What survives is the signal worth having: which embed events host applications actually subscribe to. Occurrence tracking is untouched — visual-sdk-embed-event still reports every embed event that arrives, which is the "how many events are we getting" number.

One thing to flag on the diff: three existing specs asserted the old two-argument on() call via toHaveBeenCalledWith(EmbedEvent.X, expect.anything()), which is arity-sensitive, so they failed on the extra arguments. Updated to the four-argument form in app.spec.ts, liveboard.spec.ts and pinboard.spec.ts. The behaviour they cover — which handlers a full-height embed registers — is unchanged; only the asserted call shape moved.

Full suite 1731 passed (47 files), harness 34/34.

[sastaachar]

Description rewritten to cover the embed-event half properly — event table, the typeof-except-enums rule applied to both kinds, the cost model, all four bugs fixed on the way, and the public-API notes.

While doing that I found three review comments I had not read. Two were real; addressed in aea8ca14.

handlerCount was overstating. It used every handler registered for the event type, not the ones the dispatch invoked. MessageOptions.start is a supported way to subscribe to start and end separately, so an embed registering one of each for the same event reported handlerCount: 2 on both dispatches when exactly one handler ran each time. That undermines the only thing the field is for. Now counted inside the dispatch loop. Regression test verified to fail without the fix — Expected: 1, Received: 2.

The deferred walk could describe a mutated payload. Correct, and it was a hazard I had considered and waved off — handlers get the payload by reference and run before the deferred read, so a handler normalising a field in place would change the shape being reported. Fixed by describing the payload synchronously, before the handlers, and deferring only the upload. That is the better split anyway: the walk is bounded (3 levels, 20 key paths) and cheap, while the Mixpanel call is the part that must never sit in front of a host handler. The "does not block" test still passes.

parameters?.vizId applied — belt and braces, since every current caller passes a validated object, but the line is in this diff.

Full suite 1732 passed (47 files), lint 0 errors, harness 34/34.

[sastaachar]

Two response-aware events, in d711b2eb. You were right that the response side was missing entirely — a trigger resolves with whatever the app sends back, and on(ApiIntercept, responder) means the interesting part of an interception is the answer, not the question. Neither was captured.

Event	Once per	Whole story
visual-sdk-host-event	trigger	host event, parameter shape, route, status, durationMs, responded, response shape
visual-sdk-embed-event	embed event	embed event, payload shape, handlerCount, canRespond, responded, responseTimeMs, response shape

visual-sdk-trigger-<HostEvent> is back to exactly what it was before this branch — fired at call time, no properties. The new event carries the data instead, so nothing existing changes meaning. That also retires visual-sdk-host-event-no-response: its only signal is now responded: false on the story.

Only events that can actually be answered are waited on. canRespond comes from the presence of the MessagePort — no port means triggerEventOnPort is a no-op, so a response is impossible. Those are reported as soon as the handlers have run. Events that can be answered get a five-second window, then record responded: false. So there is no timer on the vast majority of embed events, which keeps the hot path where it was.

Responses are shape-only too — responseType, responseKeys, responseShape, same summariser, never a value. A host application answering an intercept with { allow: true, answerName: 'Quarterly revenue' } reports responseKeys: ['allow','answerName'] and the name never leaves the browser; there is a test asserting exactly that.

Five new tests: a responded exchange with the response value asserted absent, an event that is never responded to (fake timers across the five-second window), an event that cannot be responded to, the response shape of a trigger, and the legacy per-event upload being untouched.

Full suite 1737 passed (47 files), lint 0 errors, browser harness 39/39 including the real 30s timeout and no customer value in any of 34 host + embed uploads.

One judgement call to flag: five seconds is a guess. It is long enough for an intercept handler that does its own fetch, short enough not to pile up. If you would rather it matched the 30s trigger timeout, or were configurable, say which.

[github-actions]

Correctness: handlerCount can undercount when a handler responds synchronously.

invokedHandlers is read by reportEmbedEvent at the moment it's called, but the count is only final once the whole forEach has finished. If an earlier handler in callbacks calls its responder synchronously (e.g. the documented EmbedEvent.ApiIntercept / OnBeforeGetVizDataIntercept pattern of calling responder(...) directly inside the callback), reportEmbedEvent fires — and sets reported = true — before later handlers in the loop have been invoked. The final reportEmbedEvent() call after the loop then becomes a no-op (reported is already true), so the upload permanently reports handlerCount as of the responding handler's position in the array, not the true number of handlers this dispatch ran.

Concretely: register two handlers for EmbedEvent.ApiIntercept where the first synchronously calls responder(...) and the second does not — handlerCount is reported as 1 even though both handlers were invoked. This is the exact metric the prior fix ("count only the handlers a dispatch ran") was meant to make accurate, so it's worth closing the gap — e.g. read invokedHandlers after the forEach completes (or snapshot it in a microtask/Promise.resolve().then() before reporting) rather than at response time.

[sastaachar]

Good catch, and an uncomfortable one — this was a regression from the commit that made handlerCount accurate in the first place. Fixed in bb33dd47.

Reporting from inside the responder meant a handler answering synchronously — the documented ApiIntercept / OnBeforeGetVizDataIntercept pattern — uploaded the event mid-loop, freezing the count at that handler's position and turning the post-loop report into a no-op. Two intercept handlers where the first answers inline reported handlerCount: 1, not 2.

The responder now records the response and reports only once the dispatch loop has finished. All three paths keep a final count: a synchronous answer reports immediately after the loop, an asynchronous one when it arrives, an absent one after the response window.

Only the first response is recorded, which matches the port — the first answer is the one the embedded app actually receives.

Regression test verified to fail without the fix: handlerCount: 1 where 2 is expected.

Full suite 1738 passed (47 files), lint 0 errors.

Worth noting for whoever reviews this: handlerCount has now been wrong twice, in two different ways, and both were caught here rather than by me. If it is not carrying its weight as a metric I would rather drop it than keep patching it — it is the least load-bearing property in the set.

[sastaachar]

Split, in 84fe0175. This PR is now PR 1: host event parameters only — title and description rewritten to match.

No existing SDK code behaviour changes. The whole diff against main is:

src/embed/ts-embed.ts                   11 +-     one call site, properties added
src/utils/hostEventTelemetry.ts        189 +      new, internal
src/utils/hostEventTelemetry.spec.ts   238 +      new
src/embed/host-event-telemetry.spec.ts  95 +      new

visual-sdk-trigger-<HostEvent> keeps its name, timing and call site — it just carries properties where it carried none.

isRegisteredBySDK is gone. The flag was broken to begin with, and threading it meant writing { start: false }, true at every internal call site just to reach a positional parameter past a default. Whether a host application listens to an embed event is better answered by counting the handlers a dispatch actually ran — no flag, and it belongs with the embed event PR anyway. V1Embed.on, liveboard.ts, app.ts and the three unrelated specs are back to untouched.

Everything else is preserved at bb33dd47 and comes back as its own PR — outcome and duration, the response story for both directions, embed event tracking, and the bug fixes. The description lists them.

Two threads above are now moot because the code they point at is not in this PR: the handlerCount comments (embed event work, PR 4) and the parameters?.vizId suggestion (host event client, PR 5). Both will be carried into the right PR rather than dropped.

Full suite 1714 passed (47 files), lint 0 errors.

[sastaachar]

Key redaction removed (33dc0e89) — the rule is about values, not key names. Keys are the SDK's own parameter names, so they go as they are. Two lodash calls became one, and the identifier regex is gone.

The whole module is now 71 lines:

const paramType = (value, key) => {
    if (value === null) return 'null';
    if (Array.isArray(value)) return `array(${value.length})`;
    if (typeof value === 'string' && ENUM_PARAMS[key]?.includes(value)) return value;
    return typeof value;
};

export const describeParams = (payload) => {
    const params = Array.isArray(payload) ? payload[0] : payload;
    return isPlainObject(params) ? mapValues(params, paramType) : {};
};

No value is ever sent unless it is an SDK enum member, which is the one thing this has to get right:

trigger(HostEvent.UpdateRuntimeFilters, [
  { columnName: 'Region', operator: EQ, values: ['west'] }
])
-> params: { columnName: 'string', operator: 'EQ', values: 'array(1)' }

Region and west never leave the browser; EQ does, because it is a token from our own contract. A test asserts exactly that, and a second one asserts a non-member under an enum key degrades to string.

The one thing I kept that is not strictly minimal is the try/catch around building and uploading. It earns its place: I verified on this branch that without it, a payload with a throwing getter makes trigger() reject with the telemetry error, so a host application loses a working host event to a telemetry side effect. Two tests cover it.

Full suite 1705 passed (47 files), lint 0 errors.

The diff against main is four files: one existing call site (+7/−1) and three new files.

[sastaachar]

Array element types instead of a count (0e16e805), and the circular question answered below.

{ columns: ['Region', 'Revenue'], points: [{ x: 1 }, null, 7], empty: [] }

-> columns: ['string', 'string']
   points:  ['object', 'null', 'number']
   empty:   []

Capped at ten elements — a filter can carry a thousand values, and a thousand copies of 'string' in a Mixpanel property is pure waste. A nested array reports as 'array' rather than expanding, so there is still no recursion anywhere in this code.

Circular payloads

Safe, and worth saying why: because nothing recurses. Only the top level is read, each value becomes a type string, and the payload itself is never handed to Mixpanel — so there is no cycle for anything to walk, including the JSON.stringify inside mixpanel-browser.

const circular = { vizId: 'd0a1' };
circular.self = circular;
circular.loop = [circular];

-> { vizId: 'string', self: 'object', loop: ['object'] }

Two tests pin it — an object holding itself and an array containing itself — rather than leaving it as a claim. This is a property the earlier recursive version had to buy with depth and path caps; dropping the recursion made it free.

A payload that throws while being read is a different matter, and that is what the try/catch covers: verified on this branch that without it, a throwing getter makes trigger() reject with the telemetry error.

Full suite 1708 passed (47 files), lint 0 errors.

[sastaachar]

Nested objects, in c89a1644. The type map mirrors the payload all the way down now:

{ points: [{ x: 1 }, null, 7] }
-> { points: [{ x: 'number' }, 'null', 'number'] }

{ filter: { column: 'Region', values: ['west'],
            applicability: { level: 'TAB', targetId: 'tab-1' } } }
-> { filter: { column: 'string', values: ['string'],
               applicability: { level: 'TAB', targetId: 'string' } } }

Recursing means cycles now have to be handled rather than being impossible, so a value already on the current path reports circular and is not followed. The visited set is per path rather than global, so a shared object referenced twice is still described twice — only an actual cycle is cut.

const circular = { vizId: 'd0a1' };
circular.self = circular;
circular.loop = [circular];
-> { vizId: 'string', self: 'circular', loop: ['circular'] }

Writing the nested test caught a gap: level had fallen out of the enum map during the earlier simplification, so applicability.level was reporting 'string' instead of 'TAB'. Applicability.level is a real enum in filter and parameter payloads — nesting is simply what made it reachable. Back in, and asserted.

Arrays stay capped at ten elements. There is no depth cap: cycle detection is what guarantees termination, so a pathologically deep payload costs a large property rather than a hang, and a stack overflow would be swallowed by the guard around the upload rather than reaching the host application. Say the word if you want a depth cap anyway.

Full suite 1709 passed (47 files), lint 0 errors.

[sastaachar]

Done in 497481a0 — the payload is serialised first, then the copy is walked. The ancestors set and the circular marker are gone, and functions and undefined values are dropped on the way through rather than needing a case each.

Behaviour verified against Node rather than assumed:

payload	result
{ cb: () => {} }	key dropped
[() => {}, 'x']	['null', 'string']
{ missing: undefined }	key dropped
{ when: new Date() }	'string'
{ m: new Map(...) }	'object' (empty)
circular / throwing getter / BigInt	throws → no parameters, reason logged

Two things worth having on the record, since this is a trade rather than a free win:

It is now the most expensive operation in this path. A full serialise and parse of the payload on every trigger, where the walk alone allocated nothing. For a filter carrying a thousand values that is a large string plus a large object, built and thrown away to read types off it.

It throws rather than degrading. A circular payload used to report { vizId: 'string', self: 'circular' } — the parts it could reach. Now it reports nothing at all. So an empty parameter map has two meanings: a host event with no parameters, or a payload that could not be serialised. A marker property would separate them if that ambiguity ever costs anything; say the word.

Neither is a blocker, and functions really are handled now, which the walk did not do — a function value would have reported 'function' instead of vanishing.

Full suite 1709 passed (47 files), lint 0 errors. Module is 93 lines.

[adityamittal3107]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces host event parameter telemetry to safely report which host events are triggered and their parameter structures (types and enum members) without leaking actual customer values. It adds the hostEventTelemetry utility along with comprehensive unit and integration tests. The review feedback highlights potential runtime serialization issues: specifically, JSON.stringify will throw a TypeError when encountering BigInt values (which are supported in runtime filters), and empty or undefined payloads can cause JSON.parse to throw a SyntaxError. Addressing these issues by adding early guards and a custom JSON replacer will ensure robust telemetry reporting.

[gemini-code-assist]

Handle BigInt values and prevent serialization failures

BigInt is a supported type in RuntimeFilter values (as defined in src/types.ts). However, JSON.stringify throws a TypeError when encountering a BigInt value. This causes the entire parameter telemetry for the host event to fail and return {}.

We can handle BigInt values gracefully by using a custom replacer function in JSON.stringify to map bigint values to a placeholder string (e.g., "__bigint__"), and then mapping that placeholder back to "bigint" in describeValue.

const describeValue = (value: unknown, key: string): ParamTypes => {
    if (value === null) {
        return 'null';
    }
    if (value === '__bigint__') {
        return 'bigint';
    }
    if (typeof value === 'string' && ENUM_PARAMS[key]?.includes(value)) {
        return value;
    }
    if (Array.isArray(value)) {
        return value.slice(0, MAX_ARRAY_TYPES).map((item) => describeValue(item, key));
    }
    if (isPlainObject(value)) {
        return mapValues(value as Record<string, unknown>, describeValue);
    }
    return typeof value;
};

[gemini-code-assist]

Guard against nullish/empty payloads and support BigInt serialization

1. Avoid throwing exceptions for control flow: When payload is undefined or [], JSON.stringify(undefined) returns undefined, which causes JSON.parse to throw a SyntaxError. This clutters debug logs with Could not describe host event payload errors for perfectly normal empty payloads. Adding early guards for nullish/empty payloads avoids this overhead.
2. Support BigInt serialization: Use a custom replacer function in JSON.stringify to serialize bigint values as "__bigint__" to prevent serialization crashes.

const replacer = (key: string, value: any) => {
    if (typeof value === 'bigint') {
        return '__bigint__';
    }
    return value;
};

export const describeParams = (payload: unknown): Record<string, ParamTypes> => {
    if (payload === undefined || payload === null) {
        return {};
    }
    const target = Array.isArray(payload) ? payload[0] : payload;
    if (target === undefined || target === null) {
        return {};
    }
    let params;
    try {
        params = JSON.parse(JSON.stringify(target, replacer));
    } catch (e) {
        logger.debug('Could not describe host event payload', e);
        return {};
    }
    return isPlainObject(params) ? describeValue(params, '') as Record<string, ParamTypes> : {};
};

[sastaachar]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces host event telemetry tracking via Mixpanel, adding a new utility hostEventTelemetry.ts and corresponding tests. It replaces direct uploadMixpanelEvent calls in TsEmbed with reportHostEvent to capture structured telemetry data while scrubbing customer values. The review feedback identifies two issues in hostEventTelemetry.ts: first, describeValue incorrectly returns 'object' for object payloads instead of the mapped object; second, describeParams does not unwrap array payloads to describe the first element's shape as expected by the tests.

[gemini-code-assist]

The describeValue function has a bug where it returns the string 'object' for any object payload, instead of returning the object with its property values replaced by their types. This causes the telemetry to report incorrect data for object-based payloads.

The function should return the modified object itself after recursively describing its properties.

        if (typeof value === 'object') {
            Object.keys(value).forEach(key => {
                if (!PRESERVED_FIELDS.includes(key)) {
                    (value as any)[key] = describeValue((value as any)[key]);
                }
            });
            return value as { [key: string]: ParamTypes };
        }

        return typeof value;

[gemini-code-assist]

The describeParams function doesn't correctly handle payloads that are arrays of objects, such as for the UpdateRuntimeFilters event. The tests indicate that for such payloads, we should describe the shape of the first object in the array, not the array itself. The current implementation describes the entire array, which leads to incorrect telemetry data for these events.

    if (Array.isArray(params) && params.length > 0 && typeof params[0] === 'object' && params[0] !== null) {
        // For array payloads of objects (like UpdateRuntimeFilters), describe the shape of the first element.
        return describeValue(params[0]);
    }
    return describeValue(params);