feat(setTelemetryOptOut): set to automatic when metrics request responds with 200#5064
Conversation
…automatic when 200 response
…telemetryoptout is not manual
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a51a78690
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4450b11468
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nataliadelmar
added
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6549b8434
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nataliadelmar
changed the title
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51c4a28b58
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b3a7ac48b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `CallDiagnosticEventsBatcher: @submitHttpRequest#${batchId}. Request successful.` | ||
| ); | ||
|
|
||
| this.handleHttpResponseStatus(res?.statusCode, payload); |
There was a problem hiding this comment.
When the browser/firewall returns the 200 blocker response, this sets telemetryOptOut but still returns res to the inherited Batcher.executeQueue() success path, which then calls handleHttpSuccess(res) and expects res.body.items to be an array. A blocker 200, including the {statusCode: 200} shape used by the new tests, has no metrics items, so submitClientEvent() rejects even though this response is now an expected opt-out signal; short-circuit or accept the marked payload for this case, and mirror the fix in the prelogin batcher.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 837bfc5e72
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `error: ${generateCommonErrorMetadata(err)}` | ||
| ); | ||
|
|
||
| this.handleHttpResponseStatus(err?.statusCode, payload); |
There was a problem hiding this comment.
When a marked batch fails with a retryable NetworkOrCORSError/statusCode 0 or no status, this call feeds that value into handleHttpResponseStatus; if telemetryOptOut is currently automatic, the helper clears it before the inherited metrics batcher re-enqueues the same payload for retry. A transient offline/CORS failure therefore removes telemetryOptOut: 'automatic' from later client events even though no non-200 HTTP response proved that the blocker condition recovered; skip this status handling for network errors, and apply the same guard in the prelogin batcher.
Useful? React with 👍 / 👎.
nataliadelmar
added
the
validated
nataliadelmar
changed the title
Thank you for your contribution! |
3 participants
When the metric submission requests responds with 200, we realise that it means that the user's browser/firewall is blocking the metrics from being sent, so we need to setTelemetryOptOut as 'automatic', only if it has not been set to manual already (manual opt out wins over automatic opt out).
We also need to set it back to undefined if we stop getting 200s after having set it to automatic.