fix(internal-plugin-metrics): log call diagnostic milestones#5045
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04e7fa4902
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| assert.isTrue( | ||
| logSpy.calledWith('Milestone,CallDiagnostic', eventName), | ||
| 'expected logger.log to be called with Milestone,CallDiagnostic and the event name' |
There was a problem hiding this comment.
Fix mismatched milestone log assertion
The implementation added above calls logger.log() with one formatted string (Milestone,CallDiagnostic: ${eventName}), but this assertion expects two separate arguments. In the internal-plugin-metrics unit suite this test is therefore always false even when the milestone log is emitted, so CI will fail until the assertion matches the actual call shape or the production log call is changed to pass two arguments.
Useful? React with 👍 / 👎.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
04e7fa4 to
6218b3b
Compare
6218b3b to
576d47f
Compare
Pull request was closed
576d47f to
3dec4d7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dec4d72a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (eventName) { | ||
| webex.logger.log('Milestone,CallDiagnostic', eventName); |
There was a problem hiding this comment.
Log milestones once per metric
When a call-diagnostic metrics POST hits a transient NetworkOrCORSError, the default metrics batcher retries by calling rerequest(), which invokes this batcher's prepareItem() again before re-enqueueing the same item. Because the milestone log is emitted inside prepareDiagnosticMetricItem(), a single client event can produce repeated Milestone,CallDiagnostic entries on each retry, seconds apart, making the client-log milestone timeline misleading; guard this per item or move the mark to the point where the event is originally created.
Useful? React with 👍 / 👎.
2f7cc97 to
d11bd48
Compare
d11bd48 to
4983b4f
Compare
Thank you for your contribution! |
COMPLETES SPARK-827015
This pull request addresses
Without well-known marks (milestones) hard to analyse client logs.
by making the following changes
This PR uses the call diagnostic events' name as milestones.
It can be extended later if needed.
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.