-
Notifications
You must be signed in to change notification settings - Fork 155
Signals sampling #1166
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
Merged
Merged
Signals sampling #1166
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
de05dd3
add signal ingestion settings
danieljackins cf6480d
Use CDN sample rate (wip)
danieljackins 705aa7e
Update and move sampling logic
danieljackins b8e7ef7
refactor debug mode
danieljackins 0d96d2d
update sample check
danieljackins 43056e5
update test
danieljackins 86094cd
Merge branch 'master' into signals-sampling
danieljackins 472334d
changeset
danieljackins b3084e4
update tests
danieljackins 3c5e011
fix
danieljackins 6df3f28
wip
danieljackins 70f0ac0
wip
danieljackins 1399435
wip
danieljackins c56b2e0
wip
danieljackins 3c98fcd
wip
danieljackins 34fbcf7
wip
danieljackins 4c9438a
remove unused type
danieljackins 93bf465
optional sampleRate
danieljackins e238b8b
add ingestion integration tests
danieljackins 46bfe4e
catch
danieljackins fe9d373
wip
danieljackins 0f14951
shorten test
danieljackins b8c6586
lint
danieljackins File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
30 changes: 30 additions & 0 deletions
30
...ges/signals/signals-integration-tests/src/tests/signals-vanilla/signals-ingestion.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import { test, expect } from '@playwright/test' | ||
| 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).toBe(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(), | ||
| ]) | ||
|
|
||
| expect(true).toBe(true) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
| */ | ||
|
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. 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) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 =)