feat: Add the plumbing for application blocking latencies client side metrics#1575
feat: Add the plumbing for application blocking latencies client side metrics#1575danieljbruce merged 315 commits intomainfrom
Conversation
for each call
…b.com/googleapis/nodejs-bigtable into 359913994-first-PR-add-infrastructure
…into 359913994-first-PR-add-infrastructure # Conflicts: # testproxy/known_failures.txt
| { | ||
| method: dataPoint.attributes.method, | ||
| client_uid: dataPoint.attributes.client_uid, | ||
| status: dataPoint.attributes.status, |
There was a problem hiding this comment.
The reason for this is because application_latencies doesn't include status, right?
So status on line 169 will be null, meaning it won't be sent? Or am I reading this wrong?
Just want to make sure I understand why status would be undefined here
| private connectivityErrorCount: number; | ||
| private streamingOperation: StreamingState; | ||
| private applicationLatencies: number[]; | ||
| private lastRowReceivedTime: Date | null; |
There was a problem hiding this comment.
is Date a safe way to store timestamps in node?
My understanding is that Date is based on the system time, so it's not monotonic, and could move backwards. A quick google search shows process.hrtime might be a better fit for this kind of thing
There was a problem hiding this comment.
We only measure deltas between dates, but just to be safe I switched to using hrtime everywhere.
| * when the operation completes. | ||
| */ | ||
| onRowReachesUser() { | ||
| const currentTime = new Date(); |
There was a problem hiding this comment.
Maybe you should check that you're in the expected state here?
There was a problem hiding this comment.
Alright. I added a guard here.
daniel-sanche
left a comment
There was a problem hiding this comment.
LGTM, but consider making the time formats more explicit
| if (projectId && this.attemptStartTime) { | ||
| const totalTime = endTime.getTime() - this.attemptStartTime.getTime(); | ||
| const totalTime = Number( | ||
| (endTime - this.attemptStartTime) / BigInt(1000000) |
There was a problem hiding this comment.
nit: it would be good to capture that this is in milliseconds somewhere. Either a comment beside this conversion, or renaming variables to something like totalMilliseconds and operationStartTimeNanos
There was a problem hiding this comment.
Also, it looks like this i a float, correct? Is that right?
There was a problem hiding this comment.
Also, it looks like this i a float, correct? Is that right?
It looks like it rounds it off for a number type.
There was a problem hiding this comment.
And I added comments / variable name changes
…into 359913994-application-blocking-latencies # Conflicts: # src/client-side-metrics/gcp-metrics-handler.ts # test/metrics-collector/metrics-collector.ts
…thub.com/googleapis/nodejs-bigtable into 359913994-application-blocking-latencies
|
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
Summary:
This PR adds the plumbing for application blocking latencies. Right now plumbing exists for all the other client side metrics that we want to support so we just add the application blocking latencies to that plumbing.
Background:
(Explain application blocking latencies)
Changes:
Important source code changes:
src/client-side-metrics/gcp-metrics-handler.ts: When operations complete this now records application latencies to the open telemetry instruments ready for export.src/client-side-metrics/metrics-handler.ts:The interface representing the operation complete data now includes application latencies.src/client-side-metrics/operation-metrics-collector.ts:Added a method to the metrics collector that is called when the row reaches the user and also code that passes application latencies to the metrics handler when the operation completes.Test changes:
test-common/expected-otel-export-input.ts:The numbers are slightly different for the fixture because of the calls toonRowReachesUserfrom the fake client.test-common/metrics-handler-fixture.ts:The numbers are slightly different for the fixture because of the calls toonRowReachesUserfrom the fake client.test/metrics-collector/metrics-collector.ts:The fake client is changed so that it makes calls toonRowReachesUserwhen it receives rows.test/metrics-collector/typical-method-call.txt: The numbers for the test output are slightly different because of the calls to onRowReachesUser.Next Steps: