Conversation
5078a52 to
b972c57
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a centralized logging service infrastructure to the mParticle Web SDK, enabling remote error and warning reporting to Rokt endpoints. The implementation includes a ReportingLogger class with rate limiting, feature flag controls, and integration with the existing Logger class.
Key changes:
- Adds ReportingLogger service with rate limiting and conditional enablement
- Integrates remote logging into the existing Logger class
- Adds support for error codes and stack trace reporting
- Configures logging and error URL endpoints via SDK configuration
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 65 comments.
Show a summary per file
| File | Description |
|---|---|
| test/jest/reportingLogger.spec.ts | Adds comprehensive test coverage for ReportingLogger and RateLimiter classes |
| src/logging/reportingLogger.ts | Implements the core ReportingLogger service with rate limiting and conditional logging |
| src/logging/logRequest.ts | Defines request body types and severity enum for logging API |
| src/logging/errorCodes.ts | Defines ErrorCodes enum for error categorization |
| src/logger.ts | Integrates ReportingLogger into existing Logger class |
| src/mp-instance.ts | Initializes ReportingLogger during SDK setup and wires configuration |
| src/sdkRuntimeModels.ts | Updates SDKLoggerApi and SDKInitConfig interfaces to support error codes |
| src/store.ts | Adds loggingUrl and errorUrl to SDKConfig interface |
| src/uploaders.ts | Extends IFetchPayload headers to support rokt-account-id header |
| src/constants.ts | Adds default logging and error URLs for both standard and CNAME configurations |
| src/identityApiClient.ts | Updates error logging to include ErrorCodes |
| src/roktManager.ts | Adds TODO comment for future enhancement |
Comments suppressed due to low confidence (1)
test/jest/reportingLogger.spec.ts:91
- Superfluous argument passed to constructor of class ReportingLogger.
logger = new ReportingLogger(baseUrl, sdkVersion, accountId, mockRateLimiter);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mattbodle
left a comment
There was a problem hiding this comment.
Could we add an integration test as part of this PR?
06e9d53 to
22114bf
Compare
|
A general question about this repo, are we using snake case or camel case for file names? Can we choose one and add to linting rules? |
| private readonly reporter: string = 'mp-wsdk'; | ||
| private readonly integration: string = 'mp-wsdk'; |
There was a problem hiding this comment.
Would these ver differ and if not should we make them one string? or have a constant and set each to the constant?
There was a problem hiding this comment.
@mattbodle what is the actual difference between these two?
fbaac50 to
732d2f0
Compare
73ec2ee to
fc4681b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 27 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.SDKConfig.flags = {}; | ||
| } | ||
|
|
There was a problem hiding this comment.
The removed comment describes important behavior about config processing. While removing outdated comments can improve code clarity, this comment explains the dual processing of config (initial and server-provided). Consider whether this context is valuable for future maintainers, or if it's documented elsewhere. If the behavior is still relevant, the comment should be updated rather than removed.
| import { IRateLimiter, RateLimiter, ReportingLogger } from '../../src/logging/reportingLogger'; | ||
| import { WSDKErrorSeverity, ErrorCodes } from '../../src/logging/types'; | ||
| import { IStore, SDKConfig } from '../../src/store'; | ||
|
|
||
| describe('ReportingLogger', () => { | ||
| let logger: ReportingLogger; | ||
| const loggingUrl = 'test-url.com/v1/log'; | ||
| const errorUrl = 'test-url.com/v1/errors'; | ||
| const sdkVersion = '1.2.3'; | ||
| let mockFetch: jest.Mock; | ||
| const accountId = '1234567890'; | ||
| let mockStore: Partial<IStore>; | ||
|
|
||
| beforeEach(() => { | ||
| mockFetch = jest.fn().mockResolvedValue({ ok: true }); | ||
| global.fetch = mockFetch; | ||
|
|
||
| mockStore = { | ||
| getRoktAccountId: jest.fn().mockReturnValue(null), | ||
| getIntegrationName: jest.fn().mockReturnValue(null) | ||
| }; | ||
|
|
||
| Object.defineProperty(window, 'location', { | ||
| writable: true, | ||
| value: { | ||
| href: 'https://e.com', | ||
| search: '' | ||
| } | ||
| }); | ||
|
|
||
| Object.defineProperty(window, 'navigator', { | ||
| writable: true, | ||
| value: { userAgent: 'ua' } | ||
| }); | ||
|
|
||
| Object.defineProperty(window, 'mParticle', { | ||
| writable: true, | ||
| value: { config: { isWebSdkLoggingEnabled: true } } | ||
| }); | ||
|
|
||
| Object.defineProperty(window, 'ROKT_DOMAIN', { | ||
| writable: true, | ||
| value: 'set' | ||
| }); | ||
|
|
||
| logger = new ReportingLogger( | ||
| { loggingUrl, errorUrl, isWebSdkLoggingEnabled: true } as SDKConfig, | ||
| sdkVersion, | ||
| mockStore as IStore, | ||
| 'test-launcher-instance-guid' | ||
| ); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('sends error logs with correct params', () => { | ||
| logger.error('msg', ErrorCodes.UNHANDLED_EXCEPTION, 'stack'); | ||
| expect(mockFetch).toHaveBeenCalled(); | ||
| const fetchCall = mockFetch.mock.calls[0]; | ||
| expect(fetchCall[0]).toContain('/v1/errors'); | ||
| const body = JSON.parse(fetchCall[1].body); | ||
| expect(body).toMatchObject({ | ||
| severity: WSDKErrorSeverity.ERROR, | ||
| code: ErrorCodes.UNHANDLED_EXCEPTION, | ||
| stackTrace: 'stack' | ||
| }); | ||
| }); | ||
|
|
||
| it('sends warning logs with correct params', () => { | ||
| mockStore.getRoktAccountId = jest.fn().mockReturnValue(accountId); | ||
| logger.warning('warn'); | ||
| expect(mockFetch).toHaveBeenCalled(); | ||
| const fetchCall = mockFetch.mock.calls[0]; | ||
| expect(fetchCall[0]).toContain('/v1/errors'); | ||
| const body = JSON.parse(fetchCall[1].body); | ||
| expect(body).toMatchObject({ | ||
| severity: WSDKErrorSeverity.WARNING | ||
| }); | ||
| expect(fetchCall[1].headers['rokt-account-id']).toBe(accountId); | ||
| }); | ||
|
|
||
| it('does not log if ROKT_DOMAIN missing', () => { | ||
| Object.defineProperty(window, 'ROKT_DOMAIN', { | ||
| writable: true, | ||
| value: undefined | ||
| }); | ||
| logger = new ReportingLogger( | ||
| { loggingUrl, errorUrl, isWebSdkLoggingEnabled: true } as SDKConfig, | ||
| sdkVersion, | ||
| mockStore as IStore, | ||
| 'test-launcher-instance-guid' | ||
| ); | ||
| logger.error('x'); | ||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does not log if feature flag and debug mode off', () => { | ||
| Object.defineProperty(window, 'mParticle', { | ||
| writable: true, | ||
| value: { config: { isWebSdkLoggingEnabled: false } } | ||
| }); | ||
| Object.defineProperty(window, 'location', { | ||
| writable: true, | ||
| value: { href: 'https://e.com', search: '' } | ||
| }); | ||
| logger = new ReportingLogger( | ||
| { loggingUrl, errorUrl, isWebSdkLoggingEnabled: false } as SDKConfig, | ||
| sdkVersion, | ||
| mockStore as IStore, | ||
| 'test-launcher-instance-guid' | ||
| ); | ||
| logger.error('x'); | ||
| expect(mockFetch).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('logs if debug mode on even if feature flag off', () => { | ||
| Object.defineProperty(window, 'mParticle', { | ||
| writable: true, | ||
| value: { config: { isWebSdkLoggingEnabled: false } } | ||
| }); | ||
| Object.defineProperty(window, 'location', { | ||
| writable: true, | ||
| value: { href: 'https://e.com', search: '?mp_enable_logging=true' } | ||
| }); | ||
| logger = new ReportingLogger( | ||
| { loggingUrl, errorUrl, isWebSdkLoggingEnabled: false } as SDKConfig, | ||
| sdkVersion, | ||
| mockStore as IStore, | ||
| 'test-launcher-instance-guid' | ||
| ); | ||
| logger.error('x'); | ||
| expect(mockFetch).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('rate limits after 3 errors', () => { | ||
| let count = 0; | ||
| const mockRateLimiter: IRateLimiter = { | ||
| incrementAndCheck: jest.fn().mockImplementation((severity) => { | ||
| return ++count > 3; | ||
| }), | ||
| }; | ||
| logger = new ReportingLogger( | ||
| { loggingUrl, errorUrl, isWebSdkLoggingEnabled: true } as SDKConfig, | ||
| sdkVersion, | ||
| mockStore as IStore, | ||
| 'test-launcher-instance-guid', | ||
| mockRateLimiter | ||
| ); | ||
|
|
||
| for (let i = 0; i < 5; i++) logger.error('err'); | ||
| expect(mockFetch).toHaveBeenCalledTimes(3); | ||
| }); | ||
|
|
||
| it('does not include account id header when account id is not set', () => { | ||
| logger.error('msg'); | ||
| expect(mockFetch).toHaveBeenCalled(); | ||
| const fetchCall = mockFetch.mock.calls[0]; | ||
| expect(fetchCall[1].headers['rokt-account-id']).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('omits user agent and url fields when navigator/location are undefined', () => { | ||
| Object.defineProperty(window, 'navigator', { | ||
| writable: true, | ||
| value: undefined | ||
| }); | ||
| Object.defineProperty(window, 'location', { | ||
| writable: true, | ||
| value: undefined | ||
| }); | ||
| logger = new ReportingLogger( | ||
| { loggingUrl, errorUrl, isWebSdkLoggingEnabled: true } as SDKConfig, | ||
| sdkVersion, | ||
| mockStore as IStore, | ||
| 'test-launcher-instance-guid' | ||
| ); | ||
| logger.error('msg'); | ||
| expect(mockFetch).toHaveBeenCalled(); | ||
| const fetchCall = mockFetch.mock.calls[0]; | ||
| const body = JSON.parse(fetchCall[1].body); | ||
| // undefined values are omitted from JSON.stringify, so these fields won't be present | ||
| expect(body).not.toHaveProperty('deviceInfo'); | ||
| expect(body).not.toHaveProperty('url'); | ||
| }); | ||
|
|
||
| it('can set store after initialization', () => { | ||
| const loggerWithoutStore = new ReportingLogger( | ||
| { loggingUrl, errorUrl, isWebSdkLoggingEnabled: true } as SDKConfig, | ||
| sdkVersion, | ||
| undefined, | ||
| 'test-launcher-instance-guid' | ||
| ); | ||
|
|
||
| const newMockStore: Partial<IStore> = { | ||
| getRoktAccountId: jest.fn().mockReturnValue(accountId), | ||
| getIntegrationName: jest.fn().mockReturnValue('custom-integration-name') | ||
| }; | ||
|
|
||
| loggerWithoutStore.setStore(newMockStore as IStore); | ||
| loggerWithoutStore.error('test error'); | ||
|
|
||
| expect(mockFetch).toHaveBeenCalled(); | ||
| const fetchCall = mockFetch.mock.calls[0]; | ||
| expect(fetchCall[1].headers['rokt-account-id']).toBe(accountId); | ||
| expect(fetchCall[1].headers['rokt-launcher-version']).toBe('custom-integration-name'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('RateLimiter', () => { | ||
| let rateLimiter: RateLimiter; | ||
| beforeEach(() => { | ||
| rateLimiter = new RateLimiter(); | ||
| }); | ||
|
|
||
| it('allows up to 10 error logs then rate limits', () => { | ||
| for (let i = 0; i < 10; i++) { | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.ERROR)).toBe(false); | ||
| } | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.ERROR)).toBe(true); | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.ERROR)).toBe(true); | ||
| }); | ||
|
|
||
| it('allows up to 10 warning logs then rate limits', () => { | ||
| for (let i = 0; i < 10; i++) { | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.WARNING)).toBe(false); | ||
| } | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.WARNING)).toBe(true); | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.WARNING)).toBe(true); | ||
| }); | ||
|
|
||
| it('allows up to 10 info logs then rate limits', () => { | ||
| for (let i = 0; i < 10; i++) { | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.INFO)).toBe(false); | ||
| } | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.INFO)).toBe(true); | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.INFO)).toBe(true); | ||
| }); | ||
|
|
||
| it('tracks rate limits independently per severity', () => { | ||
| for (let i = 0; i < 10; i++) { | ||
| rateLimiter.incrementAndCheck(WSDKErrorSeverity.ERROR); | ||
| } | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.ERROR)).toBe(true); | ||
| expect(rateLimiter.incrementAndCheck(WSDKErrorSeverity.WARNING)).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
There's no test case that verifies the info() method actually sends logs to the correct endpoint (loggingUrl vs errorUrl). The info() method calls sendLog() which uses loggingUrl, while error() and warning() call sendError() which uses errorUrl. Add a test case similar to 'sends error logs with correct params' that verifies info logs are sent to the logging endpoint with correct parameters.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/mp-instance.ts:264
- The _resetForTests method doesn't reinitialize or update the instance._ReportingLogger, which could cause test failures or inconsistent test behavior. When a test resets the SDK state and creates a new Store (line 256), the existing ReportingLogger will still reference the old Store instance, leading to stale data being used in logs. Consider either: 1) reinitializing instance._ReportingLogger similar to how Logger is reinitialized on line 255, or 2) calling instance._ReportingLogger.setStore(instance._Store) after creating the new Store to update its reference.
this._resetForTests = function(config, keepPersistence, instance, reportingLogger?: ReportingLogger) {
if (instance._Store) {
delete instance._Store;
}
instance.Logger = new Logger(config, reportingLogger);
instance._Store = new Store(config, instance);
instance._Store.isLocalStorageAvailable = instance._Persistence.determineLocalStorageAvailability(
window.localStorage
);
instance._Events.stopTracking();
if (!keepPersistence) {
instance._Persistence.resetPersistence();
}
instance._Persistence.forwardingStatsBatches.uploadsTable = {};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| this.SDKConfig.useNativeSdk = !!config.useNativeSdk; | ||
|
|
There was a problem hiding this comment.
The removal of the comment and the logLevel check is inconsistent with other config processing patterns in the codebase. The removed code at lines 351-354 checked if the config had a 'logLevel' property before applying it to SDKConfig, which is a defensive pattern used elsewhere in this function (e.g., line 333 checks hasOwnProperty for isDevelopmentMode). The logLevel configuration still exists in SDKConfig interface and is used throughout the codebase, so this removal may allow undefined logLevel values to overwrite existing values. Consider restoring the hasOwnProperty check or provide justification for why logLevel should be treated differently from other config properties.
| if (config.hasOwnProperty('logLevel')) { | |
| this.SDKConfig.logLevel = config.logLevel as LogLevelType; | |
| } |
| const roktConfig: IKitConfigs = parseConfig(config, 'Rokt', 181); | ||
| if (roktConfig) { | ||
| const accountId = roktConfig.settings?.accountId; | ||
| mpInstance._Store.setRoktAccountId(accountId); |
There was a problem hiding this comment.
The roktAccountId is set twice in the initialization flow, which could lead to confusion and potential race conditions. It's first set in completeSDKInitialization at lines 1456-1457 from the roktConfig.settings, and then set again in RoktManager.attachKit at lines 193-195 from the kit.settings. If these values differ or if the timing of kit attachment changes, this could cause inconsistent behavior. Consider removing one of these assignments or documenting why both are necessary. The initialization at line 1456-1457 appears sufficient since it happens earlier in the SDK lifecycle and uses the same source (roktConfig.settings.accountId).
| mpInstance._Store.setRoktAccountId(accountId); |
|
|
||
| if (this.logger.error) { | ||
| this.logger.error(msg); | ||
| if (code) { |
There was a problem hiding this comment.
I'm unsure if this is a good way to determine whether to report the error to our backend.
I would make it more explicit, or at a minimum change this param name to codeForReporting or something to make it obvious it's doing something additional.
There was a problem hiding this comment.
+1 on renaming param name to codeForReporting from code
|
|
||
| const accountId = this.store?.getRoktAccountId?.(); | ||
| if (accountId) { | ||
| headers['rokt-account-id'] = accountId; |
There was a problem hiding this comment.
We don't use a header keys var/enum for these?
|


Background
The Web SDK needs the ability to capture and report errors, warnings, and diagnostic information to a remote logging service for monitoring and debugging purposes. This is particularly important for Rokt integrations where we need visibility into SDK behavior in production environments. Previously, the SDK only logged to the browser console, making it difficult to diagnose issues in production.
This PR introduces a ReportingLogger service that sends logs to remote endpoints when specific conditions are met (ROKT_DOMAIN present and either a feature flag is enabled or debug mode is active via query parameter).
What Has Changed
Screenshots/Video
Checklist
Additional Notes
Reference Issue (For employees only. Ignore if you are an outside contributor)