Skip to content

Conversation

@Mercy811
Copy link
Contributor

@Mercy811 Mercy811 commented Jan 10, 2026

Summary

This PR restructures the initialization flow in browser-client.ts to create the DiagnosticsClient as early as possible, allowing it to track more data during the SDK initialization process.

Original Initialization Order

Step 2:

  1. Call useBrowserConfig() to create browser config
  2. Fetch analytics SDK remote config (configs.analyticsSDK.browserSDK)
  3. Fetch diagnostics remote config (configs.diagnostics.browserSDK) and update browserOptions
  4. Create DiagnosticsClient using values from browserOptions
  5. Assign diagnosticsClient to this.config.diagnosticsClient

New Initialization Order

Step 2:

  1. Determine fetchRemoteConfig using shouldFetchRemoteConfig(options)
  2. Fetch diagnostics config to get sample rate
  3. Create DiagnosticsClient as early as possible
  4. Call useBrowserConfig with diagnosticsClient and config
  5. Fetch analytics SDK remote config

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

Note

Moves diagnostics initialization ahead of browser config creation to capture more init telemetry and ensure consistent settings.

  • In browser-client.ts, determine fetchRemoteConfig via new shouldFetchRemoteConfig, fetch configs.diagnostics.browserSDK first, then instantiate DiagnosticsClient early with Logger and serverZone (defaulting to DEFAULT_SERVER_ZONE), and pass it plus early settings into useBrowserConfig; fetch configs.analyticsSDK.browserSDK afterward and apply via updateBrowserConfigWithRemoteConfig.
  • Add EarlyConfig and extend useBrowserConfig to accept a diagnosticsClient and early logger/serverZone/diagnostics values; simplify BrowserConfig constructor fetch-remote-config logic to remoteConfig?.fetchRemoteConfig ?? fetchRemoteConfig.
  • Introduce shouldFetchRemoteConfig(options) helper and corresponding unit tests; update tests to validate early diagnostics behavior, serverZone/enableDiagnostics overrides, and default fetchRemoteConfig behavior.

Written by Cursor Bugbot for commit 99751a8. Configure here.

@macroscopeapp
Copy link
Contributor

macroscopeapp bot commented Jan 10, 2026

Initialize diagnostics early in AmplitudeBrowser._init and pass diagnosticsClient into config.useBrowserConfig to apply remote diagnostics config before constructing BrowserConfig

Refactors client init to compute remote-config fetching via config.shouldFetchRemoteConfig, subscribe to diagnostics config first, and construct DiagnosticsClient with serverZone and loggerProvider before BrowserConfig; config.useBrowserConfig now accepts diagnosticsClient and diagnosticsConfig and embeds them at creation. Adds config.shouldFetchRemoteConfig and updates tests.

🖇️ Linked Issues

Addresses diagnostic initialization sequencing in support of AMP-146280 within the broader AMP-146279 epic and AMP-91393 initiative by refactoring early diagnostics setup.

📍Where to Start

Start in AmplitudeBrowser._init in browser-client.ts, then review config.shouldFetchRemoteConfig and the updated config.useBrowserConfig in config.ts.


Macroscope summarized 94c51ae. (Automatic summaries will resume when PR exits draft mode or review begins).

@Mercy811
Copy link
Contributor Author

bugbot run

@Mercy811
Copy link
Contributor Author

bugbot run

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no bugs!

…Config

refactor(analytics-browser): initialize diagnostics before useBrowserConfig

test: test

fix: comment
… into AMP-146280/diagnostics-cookie-storage-de-duplication
@Mercy811
Copy link
Contributor Author

Probably NX cache causes CI failed
image

I ran pnpm test locally without any error and git diff AMP-146280/diagnostics-cookie-storage-de-duplication AMP-146280/diagnostics-cookie-storage-de-duplication-2 didn't show any difference either.

So close this PR and use the #1480 instead

@Mercy811 Mercy811 closed this Jan 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants