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

stats: add a histogram version number #69944

Merged
merged 1 commit into from
Sep 10, 2021
Merged

Conversation

rytaft
Copy link
Collaborator

@rytaft rytaft commented Sep 8, 2021

This commit adds a histogram version number to the HistogramData
proto. This will allow us to identify what logic was used to construct
a particular histogram and possibly debug future issues.

Release note: None

Release justification: Low risk, high benefit change to existing
functionality.

@rytaft rytaft requested review from RaduBerinde and a team September 8, 2021 22:21
@rytaft rytaft requested a review from a team as a code owner September 8, 2021 22:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@mgartner mgartner 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 28 of 28 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)


pkg/sql/stats/histogram.go, line 39 at r1 (raw file):

// ATTENTION: When updating this field, add a brief description of what
// changed to the version history below.
const HistVersion HistogramVersion = 1

Does this need to be exported?


pkg/sql/stats/histogram.go, line 52 at r1 (raw file):

- Version: 0
- Initial histogram implementation.

Should we note here that Version 0 histograms won't have a version field?

Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @RaduBerinde)


pkg/sql/stats/histogram.go, line 39 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Does this need to be exported?

Nope! Done.


pkg/sql/stats/histogram.go, line 52 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Should we note here that Version 0 histograms won't have a version field?

Done.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @rytaft)


pkg/sql/stats/histogram.go, line 54 at r2 (raw file):

- Version: 0
- Initial histogram implementation. The version field is omitted on Version 0

[nit] I would say histogram implementations up to and including 21.1.x.

This commit adds a histogram version number to the HistogramData
proto. This will allow us to identify what logic was used to construct
a particular histogram and possibly debug future issues.

Release note: None

Release justification: Low risk, high benefit change to existing
functionality.
Copy link
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

bors r+

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


pkg/sql/stats/histogram.go, line 54 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] I would say histogram implementations up to and including 21.1.x.

Done.

@craig
Copy link
Contributor

craig bot commented Sep 10, 2021

Build succeeded:

@craig craig bot merged commit 5b40b19 into cockroachdb:master Sep 10, 2021
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.

4 participants