Repository navigation
fix(cli): concurrent deploy telemetry events share an event id - #2129
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![maintainer-for-aws[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| }); | ||
|
|
||
| test('concurrent failed DEPLOY events count every sequential failure', async () => { | ||
| // GIVEN: an in-memory state file whose reads and writes take a while, so updates overlap | ||
| let state: telemetryState.TelemetryState = {}; | ||
| const stateSpy = jest.spyOn(telemetryState, 'withTelemetryState').mockImplementation(async (block) => { | ||
| const copy = { ...state }; | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); | ||
| const ret = block(copy); | ||
| state = copy; | ||
| return ret; | ||
| }); | ||
|
|
||
| // WHEN | ||
| const error = { name: ToolkitError.name }; | ||
| const first = session.emit({ eventType: 'DEPLOY', duration: 1234, error }); | ||
| const second = session.emit({ eventType: 'DEPLOY', duration: 1234, error }); | ||
| const third = session.emit({ eventType: 'DEPLOY', duration: 1234, error }); | ||
| await first; | ||
| await second; | ||
| await third; | ||
| stateSpy.mockRestore(); | ||
|
|
||
| // THEN | ||
| const failures = clientEmitSpy.mock.calls.map(([e]) => e.counters?.sequentialDeploymentFailures); | ||
| expect(failures.sort()).toEqual([1, 2, 3]); | ||
| expect(state.sequentialDeploymentFailures).toEqual(3); | ||
| }); | ||
|
|
||
| test('DEPLOY event is still sent when the state file cannot be updated', async () => { | ||
| // GIVEN | ||
| const stateSpy = jest.spyOn(telemetryState, 'withTelemetryState').mockRejectedValue(new Error('EACCES')); | ||
|
|
||
| // WHEN | ||
| await session.emit({ eventType: 'DEPLOY', duration: 1234 }); | ||
| stateSpy.mockRestore(); | ||
|
|
||
| // THEN | ||
| expect(clientEmitSpy).toHaveBeenCalledTimes(1); | ||
| expect(clientEmitSpy.mock.calls[0][0]).toEqual(expect.objectContaining({ | ||
| event: expect.objectContaining({ eventType: 'DEPLOY' }), | ||
| })); | ||
| expect(clientEmitSpy.mock.calls[0][0].counters?.sequentialDeploymentFailures).toBeUndefined(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
⚪ Optional — Both new tests install a jest.spyOn(telemetryState, 'withTelemetryState') mock and call stateSpy.mockRestore() inline in the happy path — before the expect(...) assertions — with no afterEach/finally safety net. If an awaited emit() rejects, or an assertion placed before the restore throws, mockRestore() never runs and the mock persists into whichever test the randomized runner executes next (this repo runs tests randomized by default). That leaks global state and can cause confusing cross-test failures that do not reproduce deterministically.
Suggested change: Restore mocks in a guaranteed path: add afterEach(() => { jest.restoreAllMocks(); }); to the describe block, or wrap each test body in try { ... } finally { stateSpy.mockRestore(); }.
TelemetrySession.emit()incrementsthis.count, then for DEPLOY events awaitstrackDeployStatistics()(a read and write of the telemetry state file), and only then buildseventIdfromthis.count. With--concurrency, other events are emitted during that await, so several DEPLOY events read the same final count and are sent with the sameeventId.Rn, we see telemetry data has repeated event IDs on CLI versions after the subprocess sender, mostly DEPLOY events in the same request with different contents.
This takes the event id and timestamp at the start of
emit(), before the await.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license