-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Support clicks and custom event tracing #432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
d27cb40
b2474cf
76ac334
1a76688
357e4d7
3a31d36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,6 +127,7 @@ import { CustomSampler } from './otel/sampling/CustomSampler' | |
| import randomUuidV4 from './utils/randomUuidV4' | ||
| import { LDContext } from '@launchdarkly/js-client-sdk' | ||
| import { MaskInputOptions } from './types/record' | ||
| import { ProductAnalyticsEvents } from 'client/types/observe' | ||
|
|
||
| export const HighlightWarning = (context: string, msg: any) => { | ||
| console.warn(`Highlight Warning: (${context}): `, { output: msg }) | ||
|
|
@@ -174,6 +175,7 @@ export type HighlightClassOptions = { | |
| sendMode?: 'webworker' | 'local' | ||
| otlpEndpoint?: HighlightOptions['otlpEndpoint'] | ||
| otel?: HighlightOptions['otel'] | ||
| productAnalytics?: boolean | ProductAnalyticsEvents | ||
| contextFriendlyName?: (context: LDContext) => string | undefined | ||
| } | ||
|
|
||
|
|
@@ -634,8 +636,8 @@ export class Highlight { | |
| serviceName: | ||
| this.options?.serviceName ?? 'highlight-browser', | ||
| instrumentations: this.options?.otel?.instrumentations, | ||
| eventNames: this.options?.otel?.eventNames, | ||
| getIntegrations: () => [...this._integrations], | ||
| productAnalyticsEvents: this._productAnalyticsEvents(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double PathListener causes duplicate Navigate custom eventsMedium Severity In Additional Locations (1) |
||
| }, | ||
| sampler, | ||
| ) | ||
|
|
@@ -935,6 +937,29 @@ SessionSecureID: ${this.sessionData.sessionSecureID}`, | |
| } | ||
| } | ||
|
|
||
| private _productAnalyticsEvents(): ProductAnalyticsEvents { | ||
| const pa = this.options?.productAnalytics | ||
| if (pa === false) { | ||
| return {} | ||
| } | ||
|
|
||
| const paEvents = { | ||
| clicks: true, | ||
| pageViews: true, | ||
| trackEvents: true, | ||
| } | ||
| if (pa === undefined || pa === true) { | ||
| return paEvents | ||
| } | ||
|
|
||
| for (const event of Object.keys(pa)) { | ||
| if (pa[event as keyof ProductAnalyticsEvents] === false) { | ||
| paEvents[event as keyof ProductAnalyticsEvents] = false | ||
| } | ||
| } | ||
| return paEvents | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicated
|
||
|
|
||
| async _visibilityHandler(hidden: boolean) { | ||
| if (this.manualStopped) { | ||
| this.logger.log(`Ignoring visibility event due to manual stop.`) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ import type { | |
| OtelOptions, | ||
| } from './client' | ||
| import type { CommonOptions } from './types' | ||
| import type { EventName } from '@opentelemetry/instrumentation-user-interaction' | ||
|
|
||
| export type ObserveOptions = CommonOptions & { | ||
| /** | ||
|
|
@@ -61,10 +60,30 @@ export type ObserveOptions = CommonOptions & { | |
| * OTLP HTTP endpoint for OpenTelemetry tracing. | ||
| */ | ||
| otlpEndpoint?: string | ||
| /** | ||
| * User interaction instrumentation event names to record. | ||
| * Defaults to 'click', 'input', 'submit' window events. | ||
| */ | ||
| eventNames?: EventName[] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double checking, this is ok to remove?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Synced with @ccschmitz-launchdarkly and decided that we should remove support for this |
||
| } | ||
| /** | ||
| * Specifies whether to record product analytics events. | ||
| * @default true | ||
| */ | ||
| productAnalytics?: boolean | ProductAnalyticsEvents | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dyozie-ld for any feedback on api |
||
| } | ||
|
|
||
| export type ProductAnalyticsEvents = { | ||
| /** | ||
| * Specifies whether to record product analytics for clicks. | ||
| * Requires the use of the @opentelemetry/instrumentation-user-interaction instrumentation. | ||
| * @default true | ||
| */ | ||
| clicks?: boolean | ||
| /** | ||
| * Specifies whether to record product analytics for page views. | ||
| * Requires the use of the @opentelemetry/instrumentation-document-load instrumentation. | ||
| * @default true | ||
| */ | ||
| pageViews?: boolean | ||
| /** | ||
| * Specifies whether to record product analytics for custom events. | ||
| * @default true | ||
| */ | ||
| trackEvents?: boolean | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ import type { | |
| } from '@launchdarkly/js-client-sdk' | ||
| import { LDEvaluationReason } from '@launchdarkly/js-sdk-common/dist/cjs/api/data/LDEvaluationReason' | ||
|
|
||
| export type { Hook, LDClient } | ||
| export type { Hook, LDClient, LDContextStrict } | ||
|
|
||
| export const FEATURE_FLAG_SCOPE = 'feature_flag' | ||
| export const LD_SCOPE = 'launchdarkly' | ||
|
|
@@ -45,8 +45,12 @@ 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` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Do we want the context information under the 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree that |
||
|
|
||
| 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' | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How the name will be displayed in traces |
||
|
|
||
| export const LD_METRIC_NAME_DOCUMENT_LOAD = 'document_load' | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ import { | |
| getContextKeys, | ||
| Hook, | ||
| LD_IDENTIFY_RESULT_STATUS, | ||
| LD_TRACK_SPAN_NAME, | ||
| LDClient, | ||
| } from '../integrations/launchdarkly' | ||
| import { Observe as ObserveAPI } from '../api/observe' | ||
|
|
@@ -26,6 +27,7 @@ import { Plugin } from './common' | |
| import { | ||
| ATTR_TELEMETRY_SDK_NAME, | ||
| ATTR_TELEMETRY_SDK_VERSION, | ||
| ATTR_URL_FULL, | ||
| } from '@opentelemetry/semantic-conventions' | ||
| import { Attributes } from '@opentelemetry/api' | ||
| import { internalLog } from '../sdk/util' | ||
|
|
@@ -126,9 +128,12 @@ export class Observe extends Plugin<ObserveOptions> implements LDPlugin { | |
| hook.afterIdentify?.(hookContext, data, result) | ||
| } | ||
|
|
||
| const ldContextKeys = getContextKeys(hookContext.context) | ||
| this.observe?.setLDContextKeys(ldContextKeys) | ||
|
|
||
| if (result.status === 'completed') { | ||
| const metadata = { | ||
| ...getContextKeys(hookContext.context), | ||
| ...ldContextKeys, | ||
| key: | ||
| this.options?.contextFriendlyName?.( | ||
| hookContext.context, | ||
|
|
@@ -192,14 +197,32 @@ export class Observe extends Plugin<ObserveOptions> implements LDPlugin { | |
| []) { | ||
| hook.afterTrack?.(hookContext) | ||
| } | ||
| this.observe?.recordLog('LD.track', 'info', { | ||
|
|
||
| const trackAttrs: Attributes = { | ||
| [ATTR_URL_FULL]: window.location.href, | ||
| ...(this.observe?.getLDContextKeyAttributes() ?? {}), | ||
| ...metaAttrs, | ||
| key: hookContext.key, | ||
| value: hookContext.metricValue, | ||
| ...(typeof hookContext.data === 'object' | ||
| ? hookContext.data | ||
| : {}), | ||
| }) | ||
| } | ||
|
|
||
| const trackEventsEnabled = | ||
| this.options?.productAnalytics !== false && | ||
| (typeof this.options?.productAnalytics !== 'object' || | ||
| this.options.productAnalytics.trackEvents !== false) | ||
|
|
||
| if (trackEventsEnabled) { | ||
| this.observe?.startSpan(LD_TRACK_SPAN_NAME, (s) => { | ||
| if (s) { | ||
| s.setAttributes(trackAttrs) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| this.observe?.recordLog('LD.track', 'info', trackAttrs) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still want a log? |
||
| }, | ||
| }, | ||
| ] | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (naming): why is it
getLDContextKeyAttributesinstead ofgetLDContextKeys? Or should we call the other onesetLDContextKeyAttributes?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can look into changing this - annoyingly we have to map through the entries to prefix with
PRODUCT_ANALYTICS_CONTEXT_ATTRwhich this function does, so the data in != data out, but maybe not the best place to do so