-
Notifications
You must be signed in to change notification settings - Fork 47
fix(analytics-browser): prevent infinite Amplitude network requests #1100
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
base: main
Are you sure you want to change the base?
fix(analytics-browser): prevent infinite Amplitude network requests #1100
Conversation
… AMP-125616/fetch-hardening-xhr-support
… AMP-125616/fetch-hardening-xhr-support
- @amplitude/[email protected] - @amplitude/[email protected] - @amplitude/[email protected] - @amplitude/[email protected] - @amplitude/[email protected] - @amplitude/[email protected] - @amplitude/plugin-page-view-tracking-browser@2.3.28-fetchhardeningxhrsupport.0 - @amplitude/[email protected] - @amplitude/plugin-session-replay-react-native@0.4.2-fetchhardeningxhrsupport.0 - @amplitude/[email protected] - @amplitude/[email protected] - @amplitude/[email protected]
… AMP-125616/fetch-hardening-xhr-support
… AMP-125616/fetch-hardening-xhr-support
… AMP-125616/fetch-hardening-xhr-support
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.
Pull Request Overview
This PR prevents infinite loops in Amplitude network tracking by filtering out events that represent Amplitude’s own “[Amplitude] Network Request” calls. Key changes include:
- Adding manual browser test pages for XHR/Axios and Fetch scenarios.
- Extending
shouldTrackNetworkEvent
with a newisAmplitudeNetworkRequestEvent
check and related tests. - Adjusting core types and exports to support the new request‐type and wrapper interfaces.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test-server/browser-sdk/xhr.html | New manual test suite for XHR/Axios network tracking, including abort and various payload types |
test-server/browser-sdk/fetch.html | New manual test suite for Fetch network tracking with similar scenarios |
packages/plugin-network-capture-browser/src/track-network-event.ts | Implements amplitude‐loop detection via isAmplitudeNetworkRequestEvent and skips those events |
packages/plugin-network-capture-browser/test/autocapture-plugin/track-network-event.test.ts | Adds unit tests covering the new skip logic for “[Amplitude] Network Request” and URL‐missing cases |
packages/analytics-core/src/types/browser-config.ts | Updates networkTracking description to reflect detailed options |
packages/analytics-core/src/index.ts | Reorders exports to expose NetworkRequestEvent from the public API |
packages/analytics-core/src/network-request-event.ts | Introduces safe request/response wrappers and updated event serialization |
packages/analytics-core/src/network-observer.ts | Refactors fetch/XHR observers to use the new wrappers and centralized handling logic |
Comments suppressed due to low confidence (1)
packages/plugin-network-capture-browser/test/autocapture-plugin/track-network-event.test.ts:284
- [nitpick] Consider adding an end-to-end test for
trackNetworkEvents
itself to verify that requests flagged byshouldTrackNetworkEvent
(e.g., Amplitude loop events) are actually filtered out in the emitted analytics stream.
test('network request body contains "[Amplitude] Network Request"', () => {
@@ -8,6 +8,7 @@ import { | |||
import { filter } from 'rxjs'; | |||
import { AllWindowObservables, TimestampedEvent } from './network-capture-plugin'; | |||
import { AMPLITUDE_NETWORK_REQUEST_EVENT } from './constants'; | |||
import { IRequestWrapper } from '@amplitude/analytics-core/lib/esm/network-request-event'; |
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.
[nitpick] Avoid deep-importing from internal paths (lib/esm/...
). Instead, import IRequestWrapper
from the public package entry point to reduce brittleness.
import { IRequestWrapper } from '@amplitude/analytics-core/lib/esm/network-request-event'; | |
import { IRequestWrapper } from '@amplitude/analytics-core'; |
Copilot uses AI. Check for mistakes.
@@ -51,7 +52,10 @@ function isCaptureRuleMatch(rule: NetworkCaptureRule, hostname: string, status?: | |||
return true; | |||
} | |||
|
|||
function parseUrl(url: string) { | |||
function parseUrl(url: string | undefined) { |
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.
[nitpick] The name parseUrl
suggests returning a standard URL object but actually returns a custom result. Consider renaming to extractUrlComponents
or explicitly typing the return to improve clarity.
function parseUrl(url: string | undefined) { | |
interface UrlComponents { | |
query: string; | |
fragment: string; | |
href: string; | |
hrefWithoutQueryOrHash: string; | |
host: string; | |
} | |
function extractUrlComponents(url: string | undefined): UrlComponents | undefined { |
Copilot uses AI. Check for mistakes.
✅ No documentation updates required. |
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.
Should we ignore all requests to amplitude http v2 endpoint?
I think just requests that have |
Summary
If network tracking is configured to include amplitude.com and allows status code 200, then it can cause infinite network request calls because it would trigger an Amplitude event, and then that would trigger a network request callback, which would trigger an Amplitude event...
This checks the events, and if it contains '[Amplitude] Network Request' as an event_type, then do not track it
Checklist