Signals sampling #1166

danieljackins · 2024-10-08T16:32:53Z

I've included a changeset (psst. run yarn changeset. Read about changesets here).

changeset-bot · 2024-10-08T16:32:57Z

🦋 Changeset detected

Latest commit: b8c6586

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@segment/analytics-next	Minor
@segment/analytics-signals	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

danieljackins · 2024-10-08T16:39:32Z

@silesky does this method of cancelling signals make sense? Should the plugin be deregistered too?

silesky · 2024-10-08T16:56:02Z

packages/signals/signals/src/core/signals/signals.ts

   * - Registers custom signal generators.
   */
  async start(analytics: AnyAnalytics): Promise<void> {
+    if (


Nit: This stuff would be better suited toAnalyticsService. i.e. have an analyticsService.signalsDisabled() ?

The analyticsService class is a mean to be a facade to abstract away cdnSettings or analytics instance related logic, exposing that to our domain -- for testing, and to keep this initialization stuff tidier.

silesky · 2024-10-08T17:01:29Z

packages/signals/signals/src/core/signals/signals.ts

+    if (
+      !analytics.settings.cdnSettings.autoInstrumentationSettings ||
+      Math.random() >
+        analytics.settings.cdnSettings.autoInstrumentationSettings.sampleRate


Hey, I don't understand this Math.random() thing, maybe you could explain? I assumed we would just do:
analytics.settings.cdnSettings.autoInstrumentationSettings.sampleRate === 0?

It's a sample rate from [0,1] that we "roll against" to see if we should send signals or not (not just a boolean in the settings)

If the sample rate is 0, we just want to stop from sending signals to Segment, right? We don't want to stop processing them. This early return will abort the entire signals initialization, so we don't send signals but we also won't process (create events) from them.

The disabling of Signals completely (i.e. analytics.settings.cdnSettings.autoInstrumentationSetting) should probably not be in scope? Seems like a seperate scenario potentially?

silesky · 2024-10-08T17:03:49Z

packages/browser/src/browser/index.ts

+     * A sample rate to determine what percentage of sessions should send signals, from [0,1]
+     */
+    sampleRate: number
+  }


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

silesky · 2024-10-08T17:16:42Z

@danieljackins can we have a test / maybe an integration test for this?

silesky · 2024-10-10T19:42:47Z

packages/signals/signals/src/core/signals/settings.ts

+      this.setSignalIngestion(initialValue)
+    }
+
+    // setting ?segment_signals_debug=true will disable redaction, and set a key in local storage


Can probably delete this comment?

silesky · 2024-10-10T19:43:04Z

packages/signals/signals/src/core/signals/settings.ts

+    // 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')


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

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

silesky · 2024-10-10T19:53:00Z

packages/signals/signals/src/core/client/index.ts

    this.shouldDisableSignalRedaction =
      settings.shouldDisableSignalRedaction ?? (() => false)
+    this.signalIngestion = settings.signalIngestion ?? (() => false)
+    this.sampleRate = settings.sampleRate ?? 0


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.

silesky · 2024-10-10T19:57:30Z

packages/signals/signals/src/core/client/index.ts

    return analytics
  }

+  private shouldIngestSignals(): boolean {


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

seg-atlantis-prod · 2024-10-11T03:31:19Z

Plan Error

running git merge -q --no-ff -m atlantis-merge FETCH_HEAD: Auto-merging packages/signals/signals-integration-tests/src/helpers/base-page-object.ts
CONFLICT (content): Merge conflict in packages/signals/signals-integration-tests/src/helpers/base-page-object.ts
Auto-merging packages/signals/signals/src/core/client/__tests__/client.test.ts
Auto-merging packages/signals/signals/src/core/client/index.ts
Automatic merge failed; fix conflicts and then commit the result.
: exit status 1

silesky · 2024-10-18T14:47:40Z

packages/signals/signals/src/core/signals/settings.ts

    disallowListURLs: (string | undefined)[]
+    /**
+     * Sample rate to determine sending signals
+     */


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

silesky · 2024-10-24T15:47:02Z

packages/signals/signals-integration-tests/src/tests/signals-vanilla/signals-ingestion.test.ts

+  ])
+
+  await waitForCondition(
+    () => indexPage.signalsAPI.getEvents('interaction').length > 0,


I don't think this is and everything below it is needed -- as long as signals api has flushed, test is good!

danieljackins requested a review from silesky October 8, 2024 16:38

silesky reviewed Oct 8, 2024

View reviewed changes

silesky requested changes Oct 8, 2024

View reviewed changes

silesky reviewed Oct 8, 2024

View reviewed changes

danieljackins added 2 commits October 10, 2024 09:07

add signal ingestion settings

de05dd3

Use CDN sample rate (wip)

cf6480d

danieljackins force-pushed the signals-sampling branch from ab42130 to cf6480d Compare October 10, 2024 17:52

silesky reviewed Oct 10, 2024

View reviewed changes

silesky requested changes Oct 10, 2024

View reviewed changes

danieljackins added 4 commits October 10, 2024 17:31

Update and move sampling logic

705aa7e

refactor debug mode

b8e7ef7

update sample check

0d96d2d

update test

43056e5

danieljackins and others added 11 commits October 10, 2024 21:34

Merge branch 'master' into signals-sampling

86094cd

changeset

472334d

update tests

b3084e4

fix

3c5e011

wip

6df3f28

wip

70f0ac0

wip

1399435

wip

c56b2e0

wip

3c98fcd

wip

34fbcf7

remove unused type

4c9438a

danieljackins requested a review from silesky October 11, 2024 20:05

silesky reviewed Oct 18, 2024

View reviewed changes

danieljackins added 4 commits October 22, 2024 10:23

optional sampleRate

93bf465

add ingestion integration tests

e238b8b

catch

46bfe4e

wip

fe9d373

silesky reviewed Oct 24, 2024

View reviewed changes

danieljackins added 2 commits October 28, 2024 10:46

shorten test

0f14951

lint

b8c6586

silesky approved these changes Oct 29, 2024

View reviewed changes

danieljackins merged commit 9e6db28 into master Oct 29, 2024
7 checks passed

danieljackins deleted the signals-sampling branch October 29, 2024 16:39

github-actions bot mentioned this pull request Oct 29, 2024

Version Packages #1175

Merged

Signals sampling #1166

Signals sampling #1166

Uh oh!

Conversation

danieljackins commented Oct 8, 2024

Uh oh!

changeset-bot bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

danieljackins commented Oct 8, 2024

Uh oh!

silesky Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seg-atlantis-prod bot commented Oct 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Oct 8, 2024 •

edited

Loading

silesky Oct 8, 2024 •

edited

Loading

silesky Oct 8, 2024 •

edited

Loading

silesky Oct 10, 2024 •

edited

Loading

silesky Oct 10, 2024 •

edited

Loading