Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/browser/src/browser/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ export interface CDNSettings {
version: number
}
| {}

/**
* Settings for auto instrumentation
*/
autoInstrumentationSettings?: {
sampleRate: number
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd need a changeset for both signals and analytics =)

}

export interface AnalyticsBrowserSettings {
Expand Down
21 changes: 21 additions & 0 deletions packages/signals/signals/src/core/client/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@ export class SignalsIngestSettings {
flushInterval: number
apiHost: string
shouldDisableSignalRedaction: () => boolean
signalIngestion: () => boolean
writeKey?: string
sampleRate: number
constructor(settings: SignalsIngestSettingsConfig) {
this.flushAt = settings.flushAt ?? 5
this.apiHost = settings.apiHost ?? 'signals.segment.io/v1'
this.flushInterval = settings.flushInterval ?? 2000
this.shouldDisableSignalRedaction =
settings.shouldDisableSignalRedaction ?? (() => false)
this.signalIngestion = settings.signalIngestion ?? (() => false)
this.sampleRate = settings.sampleRate ?? 0
Copy link
Contributor

@silesky silesky Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I don't think we want to expose sampleRate publicly, as it's strictly an implementation detail -- IMO, you shouldn't be able to customize the sample rate as a setting. Can we abstract this way completely as part of this.signalsIngestion? the sampleRate should be used to completely disable signals ingestion for a current user -- so it's basically about whether or not to send signals at all for a given session -- not something we recalculate everytime a new event comes through.

}
}

Expand All @@ -23,6 +27,8 @@ export interface SignalsIngestSettingsConfig {
flushAt?: number
flushInterval?: number
shouldDisableSignalRedaction?: () => boolean
signalIngestion?: () => boolean
sampleRate?: number
}
/**
* This currently just uses the Segment analytics-next library to send signals.
Expand Down Expand Up @@ -69,10 +75,25 @@ export class SignalsIngestClient {
return analytics
}

private shouldIngestSignals(): boolean {
Copy link
Contributor

@silesky silesky Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we don't want to calculate this on a per event basis -- see comment above.

let shouldIngest = false
if (Math.random() > this.settings.sampleRate) {
// currently a no-op, but will be used once signals are sent outside of debug mode
shouldIngest = false
}
if (this.settings.signalIngestion()) {
shouldIngest = true
}
return shouldIngest
}

private sendTrackCall(signal: Signal) {
if (!this.analytics) {
throw new Error('Please initialize before calling this method.')
}
if (!this.shouldIngestSignals()) {
return
}
const disableRedaction = this.settings.shouldDisableSignalRedaction()
const data = disableRedaction
? signal.data
Expand Down
62 changes: 62 additions & 0 deletions packages/signals/signals/src/core/signals/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ export type SignalsSettingsConfig = Pick<
| 'flushAt'
| 'flushInterval'
| 'disableSignalsRedaction'
| 'signalsIngestion'
| 'sampleRate'
| 'networkSignalsAllowList'
| 'networkSignalsDisallowList'
| 'networkSignalsAllowSameDomain'
Expand All @@ -34,6 +36,7 @@ export class SignalGlobalSettings {
network: NetworkSettingsConfig

private redaction = new SignalRedactionSettings()
private ingestion = new SignalIngestionSettings()

constructor(settings: SignalsSettingsConfig) {
if (settings.maxBufferSize && settings.signalStorage) {
Expand All @@ -55,6 +58,8 @@ export class SignalGlobalSettings {
flushAt: settings.flushAt,
flushInterval: settings.flushInterval,
shouldDisableSignalRedaction: this.redaction.getDisableSignalRedaction,
signalIngestion: this.ingestion.getSignalIngestion,
sampleRate: settings.sampleRate,
}
this.sandbox = {
functionHost: settings.functionHost,
Expand All @@ -70,6 +75,7 @@ export class SignalGlobalSettings {
public update({
edgeFnDownloadURL,
disallowListURLs,
sampleRate,
}: {
/**
* The URL to download the edge function from
Expand All @@ -79,13 +85,18 @@ export class SignalGlobalSettings {
* Add new URLs to the disallow list
*/
disallowListURLs: (string | undefined)[]
/**
* The sample rate pulled from CDN settings
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this | undefined / optional, so update can be used atomically (one day?)

sampleRate: number
}): void {
edgeFnDownloadURL && (this.sandbox.edgeFnDownloadURL = edgeFnDownloadURL)
this.network.networkSignalsFilterList.disallowed.addURLLike(
...disallowListURLs.filter(<T>(val: T): val is NonNullable<T> =>
Boolean(val)
)
)
this.ingestClient.sampleRate = sampleRate
}
}

Expand Down Expand Up @@ -139,3 +150,54 @@ class SignalRedactionSettings {
return false
}
}

class SignalIngestionSettings {
private static ingestionKey = 'segment_signals_debug_ingestion_enabled'
constructor(initialValue?: boolean) {
if (typeof initialValue === 'boolean') {
this.setSignalIngestion(initialValue)
}

// setting ?segment_signals_debug=true will disable redaction, and set a key in local storage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably delete this comment?

// this setting will persist across page loads (even if there is no query string)
// in order to clear the setting, user must set ?segment_signals_debug=false
const debugModeInQs = parseDebugModeQueryString()
logger.debug('debugMode is set to true via query string')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need need to log this twice ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sorry I'm going to refactor the whole debug part still

if (typeof debugModeInQs === 'boolean') {
this.setSignalIngestion(debugModeInQs)
}
}

setSignalIngestion(shouldEnable: boolean) {
try {
if (shouldEnable) {
window.sessionStorage.setItem(
SignalIngestionSettings.ingestionKey,
'true'
)
} else {
logger.debug('Removing ingestion key from storage')
window.sessionStorage.removeItem(SignalIngestionSettings.ingestionKey)
}
} catch (e) {
logger.debug('Storage error', e)
}
}

getSignalIngestion() {
try {
const isEnabled = Boolean(
window.sessionStorage.getItem(SignalIngestionSettings.ingestionKey)
)
if (isEnabled) {
logger.debug(
`${SignalIngestionSettings.ingestionKey}=true (app. storage)`
)
return true
}
} catch (e) {
logger.debug('Storage error', e)
}
return false
}
}
3 changes: 3 additions & 0 deletions packages/signals/signals/src/core/signals/signals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ export class Signals implements ISignals {
analyticsService.instance.settings.apiHost,
analyticsService.instance.settings.cdnURL,
],
sampleRate:
analyticsService.instance.settings.cdnSettings
.autoInstrumentationSettings?.sampleRate || 0,
})

const sandbox = new Sandbox(
Expand Down
5 changes: 5 additions & 0 deletions packages/signals/signals/src/types/analytics-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,14 @@ export type EdgeFnCDNSettings = {
downloadURL: string
}

export type AutoInstrumentationCDNSettings = {
sampleRate: number
}

export interface CDNSettings {
integrations: CDNSettingsIntegrations
edgeFunction?: EdgeFnCDNSettings | { [key: string]: never }
autoInstrumentationSettings?: AutoInstrumentationCDNSettings
}

export interface SegmentEventStub {
Expand Down
10 changes: 10 additions & 0 deletions packages/signals/signals/src/types/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ export interface SignalsPluginSettingsConfig {
*/
disableSignalsRedaction?: boolean

/**
* If signals ingestion is enabled/disabled
*/
signalsIngestion?: boolean

/**
* Percentage of sessions that should be ingested
*/
sampleRate?: number

/**
* Override signals API host
* @default signals.segment.io/v1
Expand Down
Loading