Skip to content
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

Add encode time to query stats #9062

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Conversation

chencs
Copy link
Contributor

@chencs chencs commented Aug 21, 2024

What this PR does

Adds a field encode_time_seconds to query stats log messages, which records the time it takes to encode a response from the frontend.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

Does this include the time it takes queriers to marshal the results? IIRC they use protobuf to talk to the frontend, but I may be wrong. For the purposes of measuring JSON overhead in the query-frontend I think it's better to only measure at the query-frontend.

Would be nice to also get a changelog entry so we document at least somewhere what this new field means.

pkg/querier/stats/stats.go Show resolved Hide resolved
pkg/querier/stats/stats.proto Outdated Show resolved Hide resolved
@chencs
Copy link
Contributor Author

chencs commented Aug 27, 2024

Does this include the time it takes queriers to marshal the results? IIRC they use protobuf to talk to the frontend, but I may be wrong.

Yeah, the queriers use protobuf to talk to the frontend, and this doesn't include serialization time for that. I think that serialization time (among other things) would be included in the total query_wall_time_seconds, and I think encoding time is not included in query_wall_time_seconds.

For the purposes of measuring JSON overhead in the query-frontend I think it's better to only measure at the query-frontend.

Are you suggesting that we record the time somewhere other than the current changes propose? Sorry if I'm misunderstanding this; I'm pretty sure the current changes are only recording at the query-frontend.

@chencs chencs force-pushed the casie/add-encode-time-to-query-stats branch from ff470db to 98758b6 Compare August 27, 2024 23:23
@chencs chencs marked this pull request as ready for review August 27, 2024 23:24
@chencs chencs requested a review from a team as a code owner August 27, 2024 23:24
@chencs chencs force-pushed the casie/add-encode-time-to-query-stats branch from 98758b6 to 8ebfae0 Compare August 27, 2024 23:28
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

For the purposes of measuring JSON overhead in the query-frontend I think it's better to only measure at the query-frontend.

Are you suggesting that we record the time somewhere other than the current changes propose? Sorry if I'm misunderstanding this; I'm pretty sure the current changes are only recording at the query-frontend.

oh no, I was just making sure that the changes in this PR don't include the time it takes to marshal the responses in queriers. I think that's the approach we want.

LGTM

@chencs chencs merged commit ec8ca54 into main Aug 28, 2024
29 checks passed
@chencs chencs deleted the casie/add-encode-time-to-query-stats branch August 28, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants