feat: add generic network telemetry - #5146
Conversation
![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: 3ac771df13
ℹ️ 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".
| * @returns {string} | ||
| */ | ||
| function getFailureType(reason: RequestFailure, statusCode: number) { | ||
| const errorName = typeof reason?.name === 'string' ? reason.name.toLowerCase() : ''; |
There was a problem hiding this comment.
When a request-level timeout has no HTTP response, the SDK can surface it as a NetworkOrCORSError with statusCode: 0 while the original transport error is wrapped on the response body/code/message (for example the node request wrapper resolves a status-0 response with body: error). Since this only inspects the wrapper name, those timed-out SDK requests are reported as network_error instead of timeout, which undercounts the new timeout dimension; consider checking the wrapped error/code/message or timeout metadata before falling back to network_error.
Useful? React with 👍 / 👎.
| name: NETWORK_REQUEST_FAILURE_METRIC, | ||
| props: { | ||
| type: 'operational', | ||
| tags: { |
There was a problem hiding this comment.
I am not sure about the use of the tags. I did not understand codex answer....
540ffd2 to
dd62848
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd62848f06
ℹ️ 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".
| return ( | ||
| toString(wrappedError?.message) || | ||
| toString(typeof reason.body === 'string' ? reason.body : undefined) || | ||
| toString(reason.message) || |
There was a problem hiding this comment.
When a WebexHttpError has no body.message (for example an empty or non-standard 4xx/5xx body), webex-core's WebexHttpError.parse() appends the request URI and WEBEX_TRACKING_ID to reason.message. Falling back to that raw message here means each retry or failed response gets a unique errorMessage, so isSameError() won't aggregate it and tracking IDs bypass the ten-ID cap in trackingIds; please strip the appended request/tracking details or use a bounded fallback before putting it in telemetry.
Useful? React with 👍 / 👎.
dd62848 to
2f0dd84
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f0dd8479b
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| safeSetInterval(submitSummary, NETWORK_TELEMETRY_INTERVAL_MS); |
There was a problem hiding this comment.
When a Webex instance is logged out or discarded, this interval still holds the bound submitMetric callback and keeps calling submitClientMetrics every ten minutes. Because the returned timer handle is discarded, the SDK cannot clear it from an onBeforeLogout or teardown path, so old/logged-out instances can keep submitting empty summaries and stay retained; store the handle and clear it during metrics cleanup/logout.
Useful? React with 👍 / 👎.
2f0dd84 to
bf4c9bc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf4c9bc661
ℹ️ 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".
| telemetry.metricsSummary.totalRecvdResponse += 1; | ||
| getRequestMetric(telemetry, options).count_recvdResponse += 1; |
There was a problem hiding this comment.
In expired-token flows, AuthInterceptor.replay() (packages/@webex/webex-core/src/interceptors/auth.js:146-165) calls this.webex.request(options) before the original reversed response chain reaches RequestEventInterceptor (packages/@webex/http-core/src/request/index.js:36-37), so the replay emits its own request:success and then the original chain emits another success for that same replay response. Since this handler increments on every success event, those requests record two received responses but no failed 401 response/error, skewing the new failed-response and error telemetry; consider ignoring the outer replay success or recording handled 401s before auth swallows them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex recommends fix is to change interceptor ordering in webex-core, not add replay-specific logic to telemetry.
I'll consider it out of scope
bf4c9bc to
44468fe
Compare
| * @returns {string} | ||
| */ | ||
| function normalizeRouteSegment(segment: string) { | ||
| if (segment.length > 32 || segment.includes('%')) { |
There was a problem hiding this comment.
Questions:
- Are you sure this cover all cases?
- Why 32?
- Which identifier contains
%?
| count_sendRequest: number; | ||
| count_failedRequest: number; | ||
| count_recvdResponse: number; | ||
| count_failedResponse: number; |
There was a problem hiding this comment.
issue: please always use camelCase variable ad property names in JS, eg: countSendRequest
| * @param {unknown} value | ||
| * @returns {string|undefined} | ||
| */ | ||
| function toString(value: unknown) { |
There was a problem hiding this comment.
Please use type annotation only one place. Since other SDK code put all typings into the JSDoc, please do the same. But if you have typing in the docs you do not need to do them in the code
| function toString(value: unknown) { | |
| function toString(value) { |
same for the rest of the definitions
|
|
||
| return ( | ||
| toString(wrappedError?.message) || | ||
| toString(typeof reason.body === 'string' ? reason.body : undefined) || |
There was a problem hiding this comment.
toString already have a string typeof check, would it work like this instead:
| toString(typeof reason.body === 'string' ? reason.body : undefined) || | |
| toString(reason.body) || |
44468fe to
e1a068c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1a068c55d
ℹ️ 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".
| } | ||
| } | ||
|
|
||
| telemetry.errorMetrics.push(errorMetric); |
There was a problem hiding this comment.
When failures contain distinct messages—for example, a service includes a request ID or user-supplied value in each response—isSameError() treats every occurrence as unique and this unconditional push lets errorMetrics grow without limit for the entire ten-minute window. A busy failing client can therefore consume increasing memory and submit an oversized client-metrics payload that is rejected, losing the whole summary; cap the aggregate count and/or coalesce excess entries into a bounded fallback.
Useful? React with 👍 / 👎.
| identifiable information or credentials. | ||
|
|
||
| ```js | ||
| ````js |
There was a problem hiding this comment.
This four-backtick fence remains open until the four backticks after the event-flow example, so the Network request telemetry heading, its scope text, and the nested event-flow fence all render as part of the preceding JavaScript code block rather than as documentation. Close the configuration example after Webex.init(...) and use a separate fence for the event flow.
Useful? React with 👍 / 👎.
e1a068c to
a33b920
Compare
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: a33b920384
ℹ️ 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".
| return ( | ||
| toString(wrappedError?.message) || toString(reason.body) || toString(reason.message) || UNKNOWN | ||
| ); |
There was a problem hiding this comment.
When a failed request returns a large plain-text body or error message—for example, an HTML proxy error page—this path stores the entire unbounded string in errorMessage and later serializes it into the client-metrics payload. A single oversized response can therefore retain a large value for the whole reporting window and cause the complete network summary to exceed transport limits; cap or normalize error detail strings before adding them to telemetry.
Useful? React with 👍 / 👎.
| * @param options Request options containing the service or API name. | ||
| * @returns The normalized service name. | ||
| */ | ||
| function getService(options: RequestOptions): string { |
There was a problem hiding this comment.
question: did you find evidence of service names needing normalization?
| import {safeSetInterval} from '@webex/common-timers'; | ||
|
|
||
| export const NETWORK_REQUEST_SUMMARY_METRIC = 'JS_SDK_NETWORK_REQUEST_SUMMARY'; | ||
| export const NETWORK_TELEMETRY_INTERVAL_MS = 10 * 60 * 1_000; |
There was a problem hiding this comment.
question: A lot of network issues that would be useful to understand occur in the first few minutes of a call. We might not get any telemetry from these as the call will be over before 10 minutes. What is the plan for those calls? This interval is probably better as a configurable default, but also we might want a manual submission path, so consuming applications can submit telemetry when they think it is appropriate e.g. after join failure or before call end.
There was a problem hiding this comment.
Maybe that could be an improvement for the next iteration ?
| } | ||
|
|
||
| const start = options.$timings?.networkStart; | ||
| const end = options.$timings?.networkEnd; |
There was a problem hiding this comment.
thought: if you are computing latency based on the start and end times, this could be reported in some way. Even if it didn't timeout, high network latency can still be bad
| return; | ||
| } | ||
|
|
||
| this.networkTelemetry = createNetworkTelemetryCollector({ |
There was a problem hiding this comment.
issue: this whole feature should have a config enable flag which defaults to false
| * @returns Nothing. | ||
| */ | ||
| function stop(): void { | ||
| clearInterval(telemetryInterval); |
There was a problem hiding this comment.
suggestion: might want to send the network telemetry on stop, or at least try to. It is set up to stop on before logout, so maybe you can't send it at that point, but if you don't try, you might always lose the last up to 10 minutes of telemetry
a33b920 to
300d4a4
Compare
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: 300d4a4936
ℹ️ 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".
| * @returns | ||
| */ | ||
| onBeforeLogout() { | ||
| this.stopNetworkTelemetry(); |
There was a problem hiding this comment.
When logout occurs with a non-empty partial window, this hook returns immediately even though stop() initiates an asynchronous submitClientMetrics() call. The client-metrics batcher waits up to batcherWait before issuing its request, while WebexCore.logout() proceeds to clear storage and invalidate credentials as soon as this hook resolves, so the documented shutdown summary can be sent after authentication has been torn down and be lost. Return the submission promise through stop() and stopNetworkTelemetry() so logout waits for the flush.
Useful? React with 👍 / 👎.
| metric.totalNetworkDurationMs += duration; | ||
| metric.networkDurationsMs.push(duration); |
There was a problem hiding this comment.
For a high-volume endpoint, every completed request appends another duration that remains resident for the full ten-minute window, after which the entire array is copied and sorted to calculate each percentile. Unlike the submitted aggregate, memory use and boundary-time CPU therefore grow with raw request volume; a busy SDK can cause a large allocation and event-loop stall every reporting interval. Use a bounded histogram, streaming quantile estimator, or capped reservoir instead of retaining every sample.
Useful? React with 👍 / 👎.
3 participants
COMPLETES #SPARK-821340
This pull request addresses
Today, we have nothing in the logs that helps us track volume of requests made by the app and to where they go, we probably want a general request logger to be able to track this information for a session
by making the following changes
Add a generic telemetry on network requests
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.