Skip to content

adapter: Add result_rows_first_to_last_byte_seconds metric #32668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ggevay
Copy link
Contributor

@ggevay ggevay commented Jun 5, 2025

Add histogram metric for time between first result row returned and last result row returned. (This is part of #32504, although it's not yet mentioned in the design doc.)

This is more involved than my other similar PRs from yesterday, because the control flow for returning results is a bit complex, with several variations in various situations, see tests. (I wanted to also add a test for a FETCH that has a timeout, but it seems FETCH's timeout is currently not working: https://github.com/MaterializeInc/database-issues/issues/9354 )

Nightly: https://buildkite.com/materialize/nightly/builds/12231

Motivation

  • This PR adds a feature that has not yet been specified.

Tips for reviewer

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@ggevay ggevay force-pushed the first-last-byte-returned-metric branch 2 times, most recently from 334ca29 to 4dc8fd8 Compare June 5, 2025 19:55
@ggevay ggevay force-pushed the first-last-byte-returned-metric branch from 4dc8fd8 to 94324d6 Compare June 5, 2025 20:06
0.000_008, 0.000_016, 0.000_032, 0.000_064, 0.000_128, 0.000_256, 0.000_512, 0.001, 0.002,
0.004, 0.008, 0.016, 0.032, 0.064, 0.128, 0.256, 0.512, 1.0, 2.0, 4.0, 8.0, 16.0, 32.0,
0.004, 0.008, 0.016, 0.032, 0.064, 0.128, 0.256, 0.512, 1.0, 2.0, 4.0, 8.0, 16.0, 32.0, 64.0,
128.0, 256.0, 512.0, 1024.0, 2048.0, 4096.0, 8192.0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I've checked that the only existing metric that this change affected is one of the metrics that I added yesterday: mz_pgwire_message_processing_seconds. I didn't know that there is this limitation when I added that metric, so what I put into the histogram_seconds_buckets was not taking effect. Now this is fixed.)

@ggevay ggevay marked this pull request as ready for review June 5, 2025 20:14
@ggevay ggevay requested a review from a team as a code owner June 5, 2025 20:14
@ggevay ggevay requested a review from aljoscha June 5, 2025 20:14
@ggevay ggevay added the A-ADAPTER Topics related to the ADAPTER layer label Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ADAPTER Topics related to the ADAPTER layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant