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

sql: add query level execution stats to sampled query log #87527

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

THardy98
Copy link

@THardy98 THardy98 commented Sep 7, 2022

Partially addresses: #84729

This change adds:

  • network bytes sent
  • maximum memory usage
  • maximum disk usage
  • KV bytes read
  • KV rows read
  • network messages

fields to the SampledQuery telemetry log.

Release justification: low risk, high benefit changes to existing functionality

Release note (sql change): This change adds::

  • network bytes sent
  • maximum memory usage
  • maximum disk usage
  • KV bytes read
  • KV rows read
  • network messages fields to the SampledQuery telemetry log.

@THardy98 THardy98 requested a review from a team September 7, 2022 18:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@maryliag maryliag requested a review from a team September 7, 2022 19:45
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @THardy98)


pkg/util/log/eventpb/telemetry.proto line 147 at r1 (raw file):

  // The number of network bytes sent by nodes for this query.
  int64 network_bytes_sent = 39 [(gogoproto.jsontag) = ',omitempty'];

looks like there will be some merge conflicts with https://github.com/cockroachdb/cockroach/pull/87508/files
you and @xinhaoz should coordinate on merges (or one can already rebase on the other)

@THardy98 THardy98 force-pushed the add_exec_stats_to_sampled_query branch from ad84bed to 88af984 Compare September 7, 2022 20:02
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @THardy98)

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

you're missing some generated code, you need to run dev gen to create the docs

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @THardy98)

Partially addresses: cockroachdb#84729

This change adds:
- network bytes sent
- maximum memory usage
- maximum disk usage
- KV bytes read
- KV rows read
- network messages

fields to the `SampledQuery` telemetry log.

Release justification: low risk, high benefit changes to existing
functionality

Release note (sql change): This change adds::
- network bytes sent
- maximum memory usage
- maximum disk usage
- KV bytes read
- KV rows read
- network messages fields to the `SampledQuery` telemetry log.
@THardy98 THardy98 force-pushed the add_exec_stats_to_sampled_query branch from 88af984 to c546e33 Compare September 8, 2022 13:44
@THardy98
Copy link
Author

THardy98 commented Sep 8, 2022

CI failed essential Bazel CI on flakey eslint.sh test. Going to bors.

@THardy98
Copy link
Author

THardy98 commented Sep 8, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 8, 2022

Build succeeded:

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.

3 participants