Skip to content

Conversation

@ggevay
Copy link
Contributor

@ggevay ggevay commented Jun 4, 2025

This PR implements the next part of #32504, namely the following two metrics:

  • parsing time
  • processing time for the various pgwire messages to cover the Extended Query flow.

(I'm planning one more PR, which would add a metric for time between first result byte sent and last result byte sent. That one is somewhat less important, so I'd like to get just the above two merged first.)

Motivation

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 lifecycle-metrics-1 branch from 0584a51 to c550cdc Compare June 4, 2025 15:25
@ggevay ggevay marked this pull request as ready for review June 4, 2025 15:28
@ggevay ggevay requested a review from a team as a code owner June 4, 2025 15:28
@ggevay ggevay requested a review from aljoscha June 4, 2025 15:28
@ggevay ggevay force-pushed the lifecycle-metrics-1 branch from c550cdc to 84d8b02 Compare June 4, 2025 15:33
Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

Looks good, the test might get flaky if the machine we're running on is very overloaded. But we can fix that once we get there, I think.

@ggevay
Copy link
Contributor Author

ggevay commented Jun 4, 2025

Ok, thank you!

the test might get flaky if the machine we're running on is very overloaded.

There is another test, workflow_test_optimizer_metrics, which uses the same idioms (assert 0 < time < 10 for some metrics related to query processing time), and I've never seen it flake, so this should hopefully be rare.

@ggevay ggevay merged commit 3bf3b61 into MaterializeInc:main Jun 4, 2025
82 checks passed
@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.

2 participants