feat: Support clicks and custom event tracing#432
feat: Support clicks and custom event tracing#432SpennyNDaJets wants to merge 6 commits intomainfrom
Conversation
| export const LD_INITIALIZE_EVENT = '$ld:telemetry:session:init' | ||
| export const LD_TRACK_EVENT = '$ld:telemetry:track' | ||
| export const LD_TRACK_SPAN_NAME = 'launchdarkly.track' |
There was a problem hiding this comment.
How the name will be displayed in traces
| }) | ||
| } | ||
|
|
||
| this.observe?.recordLog('LD.track', 'info', trackAttrs) |
There was a problem hiding this comment.
Do we still want a log?
| private _productAnalyticsEvents(): ProductAnalyticsEvents { | ||
| const pa = this._options?.productAnalytics | ||
| if (pa === false) { | ||
| return {} |
There was a problem hiding this comment.
Disabling product analytics still tracks clicks and page views
Medium Severity
When productAnalytics is false, _productAnalyticsEvents() returns {} (empty object). Downstream consumers check config.productAnalyticsEvents?.clicks !== false and config.productAnalyticsEvents?.pageViews !== false, which both evaluate to undefined !== false → true. This means click tracking and page view tracking remain enabled even when the user explicitly opts out by setting productAnalytics: false. The empty object needs to explicitly set clicks: false, pageViews: false, and trackEvents: false for the opt-out to propagate correctly.
Additional Locations (1)
There was a problem hiding this comment.
This is correct - they are on by default
| } | ||
| } | ||
| return paEvents | ||
| } |
There was a problem hiding this comment.
Duplicated _productAnalyticsEvents method across two files
Low Severity
_productAnalyticsEvents() is implemented identically in both ObserveSDK and the Highlight client class. This duplication means any bug fix (such as the productAnalytics: false issue) needs to be applied in two places, increasing the risk of divergent behavior over time. Extracting this into a shared utility function would reduce maintenance burden.
Additional Locations (1)
There was a problem hiding this comment.
Confused if it needs to be implemented in the highlight class but put it there to be safe
| @@ -45,8 +45,13 @@ export const FEATURE_FLAG_VARIATION_INDEX_ATTR = `${FEATURE_FLAG_SCOPE}.result.v | |||
| export const FEATURE_FLAG_APP_ID_ATTR = `${LD_SCOPE}.application.id` | |||
| export const FEATURE_FLAG_APP_VERSION_ATTR = `${LD_SCOPE}.application.version` | |||
|
|
|||
| export const PRODUCT_ANALYTICS_SCOPE = 'product_analytics' | |||
| export const PRODUCT_ANALYTICS_CONTEXT_ATTR = `${PRODUCT_ANALYTICS_SCOPE}.context_keys` | |||
There was a problem hiding this comment.
question: Do we want the context information under the product_analytics scope? At least in a spans attributes.
I imagine the context being a more generic attribute which we can add to spans across various use cases, so I prefer not locking it into PA even though it fits for this instance.
Maybe context.context_keys.<key>?
There was a problem hiding this comment.
Yeah, open to changing this. I wasn't sure how important this was to be unique for processing the traces into the "fact tables", or how overloaded context may be (doesn't look like we are using it currently). May see if @mayberryzane has a preference?
There was a problem hiding this comment.
agree that context probably makes more sense than product_analytics since it's not PA specific
| * User interaction instrumentation event names to record. | ||
| * Defaults to 'click', 'input', 'submit' window events. | ||
| */ | ||
| eventNames?: EventName[] |
There was a problem hiding this comment.
Double checking, this is ok to remove?
There was a problem hiding this comment.
Synced with @ccschmitz-launchdarkly and decided that we should remove support for this
| const span = this.tracer.startSpan(LD_PAGE_VIEW_SPAN_NAME, { | ||
| attributes: { | ||
| [SemanticAttributes.ATTR_URL_FULL]: currentUrl, | ||
| 'page_view.previous_url': previousUrl, |
There was a problem hiding this comment.
Is this useful?
| @@ -168,4 +168,6 @@ export interface Observe { | |||
| environmentMetadata: LDPluginEnvironmentMetadata, | |||
| ): void | |||
| getHooks?(metadata: LDPluginEnvironmentMetadata): Hook[] | |||
| setLDContextKeys(contextKeys: Attributes): void | |||
| getLDContextKeyAttributes(): Attributes | undefined | |||
There was a problem hiding this comment.
nit (naming): why is it getLDContextKeyAttributes instead of getLDContextKeys? Or should we call the other one setLDContextKeyAttributes?
There was a problem hiding this comment.
I can look into changing this - annoyingly we have to map through the entries to prefix with PRODUCT_ANALYTICS_CONTEXT_ATTR which this function does, so the data in != data out, but maybe not the best place to do so
| * Specifies whether to record product analytics events. | ||
| * @default true | ||
| */ | ||
| productAnalytics?: boolean | ProductAnalyticsEvents |
| this._popstateHandler = () => plugin._checkLocation() | ||
| window.addEventListener('popstate', this._popstateHandler) | ||
|
|
||
| this._pollInterval = setInterval(() => plugin._checkLocation(), 300) |
There was a problem hiding this comment.
why is polling necessary in addition to patching the history methods?
There was a problem hiding this comment.
I updated to sync with the session replay logic but think the js-core logic had some holes in the patching
/** The location is watched via polling and popstate events because it is possible to miss
* navigation at certain points with just popstate. It is also to miss events with polling
* because they can happen within the polling interval.
* Details on when popstate is called:
* https://developer.mozilla.org/en-US/docs/Web/API/Window/popstate_event#when_popstate_is_sent
*/
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| } | ||
| } | ||
| return paEvents | ||
| } |
There was a problem hiding this comment.
Duplicated _productAnalyticsEvents method across two files
Medium Severity
The _productAnalyticsEvents() method is identically duplicated in both client/index.tsx and sdk/observe.ts. This duplication increases the risk that a bug fix (like the productAnalytics: false issue) gets applied in one location but missed in the other. Extracting this into a shared utility would avoid that.
Additional Locations (1)
| instrumentations: this.options?.otel?.instrumentations, | ||
| eventNames: this.options?.otel?.eventNames, | ||
| getIntegrations: () => [...this._integrations], | ||
| productAnalyticsEvents: this._productAnalyticsEvents(), |
There was a problem hiding this comment.
Double PathListener causes duplicate Navigate custom events
Medium Severity
In client/index.tsx, setupBrowserTracing now receives productAnalyticsEvents (defaulting to all enabled), which causes a new LocationChangeInstrumentation to be registered. This instrumentation uses PathListener, which patches history.pushState/replaceState and dispatches locationchange events. But client/index.tsx already registers its own PathListener at line 1072. Two PathListener instances each wrap history methods, so every navigation dispatches locationchange twice. The LocationChangeInstrumentation deduplicates via _lastUrl, but the existing navigate callback has no dedup, producing duplicate "Navigate" custom events.


Summary
Support tracing for the following product analytics events
Make sure to add the LD context to each of these traces
Note:
How did you test this change?
https://www.loom.com/share/e112659086f94a228279a97cc2b690b7
Are there any deployment considerations?
Breaking change: removal of event_names
Note
Medium Risk
Introduces new tracing behavior (click/page-view/track spans) and removes the
otel.eventNamesconfiguration, which is a breaking API change and could affect downstream instrumentation expectations.Overview
Adds product analytics tracing across the observability SDK: click interactions, SPA page views, and
LD.tracknow emit dedicated spans and include the active LaunchDarkly context keys as span attributes.Replaces the old
otel.eventNamesknob with a newproductAnalyticsoption (boolean or per-event toggles) and wires this throughObserveSDK/HighlightOTEL setup;LD.trackadditionally starts alaunchdarkly.trackspan when enabled.Updates the e2e React Router app with new
LDClientdemo routes (auto-start + manual-start) and points plugin backends/OTLP endpoints to staging to exercise the new spans.Written by Cursor Bugbot for commit 3a31d36. This will update automatically on new commits. Configure here.