-
Notifications
You must be signed in to change notification settings - Fork 136
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
Signals sampling #1166
Signals sampling #1166
Changes from 21 commits
de05dd3
cf6480d
705aa7e
b8e7ef7
0d96d2d
43056e5
86094cd
472334d
b3084e4
3c5e011
6df3f28
70f0ac0
1399435
c56b2e0
3c98fcd
34fbcf7
4c9438a
93bf465
e238b8b
46bfe4e
fe9d373
0f14951
b8c6586
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 |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'@segment/analytics-next': minor | ||
'@segment/analytics-signals': minor | ||
--- | ||
|
||
Add sampling logic and block non debug traffic |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import { test, expect } from '@playwright/test' | ||
import { waitForCondition } from '../../helpers/playwright-utils' | ||
import { IndexPage } from './index-page' | ||
|
||
const indexPage = new IndexPage() | ||
|
||
const basicEdgeFn = `const processSignal = (signal) => {}` | ||
|
||
test('ingestion not enabled -> will not send the signal', async ({ page }) => { | ||
await indexPage.loadAndWait(page, basicEdgeFn, { | ||
enableSignalsIngestion: false, | ||
}) | ||
|
||
await indexPage.fillNameInput('John Doe') | ||
await indexPage.waitForSignalsApiFlush().catch(() => { | ||
expect(true) | ||
}) | ||
}) | ||
|
||
test('ingestion enabled -> will send the signal', async ({ page }) => { | ||
await indexPage.loadAndWait(page, basicEdgeFn, { | ||
enableSignalsIngestion: true, | ||
}) | ||
|
||
await Promise.all([ | ||
indexPage.fillNameInput('John Doe'), | ||
indexPage.waitForSignalsApiFlush(), | ||
]) | ||
|
||
await waitForCondition( | ||
() => indexPage.signalsAPI.getEvents('interaction').length > 0, | ||
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. I don't think this is and everything below it is needed -- as long as signals api has flushed, test is good! |
||
{ errorMessage: 'No interaction signals found' } | ||
) | ||
|
||
const interactionSignals = indexPage.signalsAPI.getEvents('interaction') | ||
expect(interactionSignals.length > 0) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ export type SignalsSettingsConfig = Pick< | |
| 'flushAt' | ||
| 'flushInterval' | ||
| 'disableSignalsRedaction' | ||
| 'enableSignalsIngestion' | ||
| 'networkSignalsAllowList' | ||
| 'networkSignalsDisallowList' | ||
| 'networkSignalsAllowSameDomain' | ||
|
@@ -33,7 +34,8 @@ export class SignalGlobalSettings { | |
ingestClient: SignalsIngestSettingsConfig | ||
network: NetworkSettingsConfig | ||
|
||
private redaction = new SignalRedactionSettings() | ||
private sampleSuccess = false | ||
private signalsDebug = new SignalsDebugSettings() | ||
|
||
constructor(settings: SignalsSettingsConfig) { | ||
if (settings.maxBufferSize && settings.signalStorage) { | ||
|
@@ -42,8 +44,9 @@ export class SignalGlobalSettings { | |
) | ||
} | ||
|
||
this.redaction = new SignalRedactionSettings( | ||
settings.disableSignalsRedaction | ||
this.signalsDebug = new SignalsDebugSettings( | ||
settings.disableSignalsRedaction, | ||
settings.enableSignalsIngestion | ||
) | ||
|
||
this.signalBuffer = { | ||
|
@@ -54,7 +57,17 @@ export class SignalGlobalSettings { | |
apiHost: settings.apiHost, | ||
flushAt: settings.flushAt, | ||
flushInterval: settings.flushInterval, | ||
shouldDisableSignalRedaction: this.redaction.getDisableSignalRedaction, | ||
shouldDisableSignalsRedaction: | ||
this.signalsDebug.getDisableSignalsRedaction, | ||
shouldIngestSignals: () => { | ||
if (this.signalsDebug.getEnableSignalsIngestion()) { | ||
return true | ||
} | ||
if (!this.sampleSuccess) { | ||
return false | ||
} | ||
return false | ||
}, | ||
} | ||
this.sandbox = { | ||
functionHost: settings.functionHost, | ||
|
@@ -70,6 +83,7 @@ export class SignalGlobalSettings { | |
public update({ | ||
edgeFnDownloadURL, | ||
disallowListURLs, | ||
sampleRate, | ||
}: { | ||
/** | ||
* The URL to download the edge function from | ||
|
@@ -79,58 +93,80 @@ export class SignalGlobalSettings { | |
* Add new URLs to the disallow list | ||
*/ | ||
disallowListURLs: (string | undefined)[] | ||
/** | ||
* Sample rate to determine sending signals | ||
*/ | ||
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. 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) | ||
) | ||
) | ||
if (sampleRate && Math.random() <= sampleRate) { | ||
this.sampleSuccess = true | ||
} | ||
} | ||
} | ||
|
||
class SignalRedactionSettings { | ||
class SignalsDebugSettings { | ||
private static redactionKey = 'segment_signals_debug_redaction_disabled' | ||
constructor(initialValue?: boolean) { | ||
if (typeof initialValue === 'boolean') { | ||
this.setDisableSignalRedaction(initialValue) | ||
private static ingestionKey = 'segment_signals_debug_ingestion_enabled' | ||
constructor(disableRedaction?: boolean, enableIngestion?: boolean) { | ||
if (typeof disableRedaction === 'boolean') { | ||
this.setDebugKey(SignalsDebugSettings.redactionKey, disableRedaction) | ||
} | ||
if (typeof enableIngestion === 'boolean') { | ||
this.setDebugKey(SignalsDebugSettings.ingestionKey, enableIngestion) | ||
} | ||
|
||
// setting ?segment_signals_debug=true will disable redaction, and set a key in local storage | ||
// setting ?segment_signals_debug=true will disable redaction, enable ingestion, and set keys in local storage | ||
// 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') | ||
if (typeof debugModeInQs === 'boolean') { | ||
this.setDisableSignalRedaction(debugModeInQs) | ||
this.setDebugKey(SignalsDebugSettings.redactionKey, debugModeInQs) | ||
this.setDebugKey(SignalsDebugSettings.ingestionKey, debugModeInQs) | ||
} | ||
} | ||
|
||
setDisableSignalRedaction(shouldDisable: boolean) { | ||
setDebugKey(key: string, enable: boolean) { | ||
try { | ||
if (shouldDisable) { | ||
window.sessionStorage.setItem( | ||
SignalRedactionSettings.redactionKey, | ||
'true' | ||
) | ||
if (enable) { | ||
window.sessionStorage.setItem(key, 'true') | ||
} else { | ||
logger.debug('Removing redaction key from storage') | ||
window.sessionStorage.removeItem(SignalRedactionSettings.redactionKey) | ||
logger.debug(`Removing debug key ${key} from storage`) | ||
window.sessionStorage.removeItem(key) | ||
} | ||
} catch (e) { | ||
logger.debug('Storage error', e) | ||
} | ||
} | ||
|
||
getDisableSignalsRedaction() { | ||
try { | ||
const isEnabled = Boolean( | ||
window.sessionStorage.getItem(SignalsDebugSettings.redactionKey) | ||
) | ||
if (isEnabled) { | ||
logger.debug(`${SignalsDebugSettings.redactionKey}=true (app. storage)`) | ||
return true | ||
} | ||
} catch (e) { | ||
logger.debug('Storage error', e) | ||
} | ||
return false | ||
} | ||
|
||
getDisableSignalRedaction() { | ||
getEnableSignalsIngestion() { | ||
try { | ||
const isDisabled = Boolean( | ||
window.sessionStorage.getItem(SignalRedactionSettings.redactionKey) | ||
const isEnabled = Boolean( | ||
window.sessionStorage.getItem(SignalsDebugSettings.ingestionKey) | ||
) | ||
if (isDisabled) { | ||
logger.debug( | ||
`${SignalRedactionSettings.redactionKey}=true (app. storage)` | ||
) | ||
if (isEnabled) { | ||
logger.debug(`${SignalsDebugSettings.ingestionKey}=true (app. storage)`) | ||
return true | ||
} | ||
} catch (e) { | ||
|
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.
We'd need a changeset for both signals and analytics =)