This repository was archived by the owner on Mar 26, 2026. It is now read-only.
feat: fix plumbing for application latencies to use timed stream#1646
Closed
danieljbruce wants to merge 13 commits into
Closed
feat: fix plumbing for application latencies to use timed stream#1646danieljbruce wants to merge 13 commits into
danieljbruce wants to merge 13 commits into
Conversation
be customized
Removes the onRowReachesUser method and instead uses the userStream to record application latency. This simplifies the API and makes the metric more accurate. The following files were modified: - src/client-side-metrics/gcp-metrics-handler.ts - src/client-side-metrics/metrics-handler.ts - src/client-side-metrics/operation-metrics-collector.ts - test/metrics-collector/metrics-collector.ts - test/metrics-collector/typical-method-call.txt - test-common/expected-otel-export-input.ts - test-common/metrics-handler-fixture.ts
the analytics
product-auto-label
Bot
added
size: l
danieljbruce
commented
Jul 24, 2025
| if (options?.transformHook) { | ||
| options?.transformHook(event, _encoding, callback); | ||
| } | ||
| callback(null, event); |
Contributor
Author
There was a problem hiding this comment.
In the userStreams the callback of the transform function is used in several places and sometimes does not pass an event. So this change relies on the transformHook to use the callback in the right places and offers us the flexibility that we need.
danieljbruce
commented
Jul 24, 2025
| objectMode: true, | ||
| highWaterMark: 0, | ||
| readableHighWaterMark: 0, // We need to disable readside buffering to allow for acceptable behavior when the end user cancels the stream early. | ||
| writableHighWaterMark: 0, // We need to disable writeside buffering because in nodejs 14 the call to _transform happens after write buffering. This creates problems for tracking the last seen row key. |
Contributor
Author
There was a problem hiding this comment.
I think this doesn't change the way the stream works, but its nicer if we have the comments here that we had for readRows.
danieljbruce
commented
Jul 24, 2025
| }); | ||
| } | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
This is now necessary because we have to tell the TimedStream to use the callback in the transformHook now.
product-auto-label
Bot
added
size: m
Contributor
Author
|
This PR was a building block for #1647. Since the PR is merged this no longer needs review and we can close this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Application latencies are recorded in the metrics collector now by reading the data that gets collected on the TimedStream object. This PR prepares all the plumbing so that in a follow-up PR, when we pass in the TimedStream for readRows and other endpoints that the TimedStream will send the right application latencies measurements to Google Cloud monitoring dashboard.
Impact
This PR means that in a follow-up PR, all we have to do is pass in the timed stream that the user has access to into the metrics collector and we can expect this timed stream to send the application blocking latencies to the Google Cloud Monitoring dashboard correctly.
Testing
test-common/expected-otel-export-input.ts: The numbers for the test fixtures in this file have changed slightly because they depend on the values in metrics-handler-fixture.ts and those values have changed.test-common/metrics-handler-fixture.ts: The numbers have changed for the metrics collected in this file because test/metrics-collector/metrics-collector.ts test simulates events that now exclude the event that occurs with a call to onRowReachesUser.test/metrics-collector/metrics-collector.ts: This test has changed so that the fakeMethod doesn't call onRowReachesUser anymore because onRowReachesUser doesn't exist and instead a fake application latencies value is provided by a fake user stream.test/metrics-collector/typical-method-call.txt: This fixture needs to change because it records the order of events that have occurred intest/metrics-collector/metrics-collector.tsand the order of events that have occurred intest/metrics-collector/metrics-collector.tshas changed.test/timed-stream.ts: The user stream in this method now explicitly calls a callback in its transformHook method.Additional Information
Next steps: