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

ts: include histogram quantiles in tsdump #69469

Merged
merged 1 commit into from
Aug 28, 2021

Conversation

tbg
Copy link
Member

@tbg tbg commented Aug 27, 2021

cockroach debug tsdump previously silently did not return metrics
backed by histograms. This is for technical reasons related to the
bookkeeping of metrics names and is rectified here by requiring some
extra tagging of metrics that are histograms so that they can be picked
up by tsdump. It's not pretty, but pragmatic: it works and it'll be
clear to anyone adding a histogram in the future how to proceed, even if
they may wonder why things work in such a roundabout manner (and if
they're curious about that, the relevant issues are linked in comments
as well).

I also renamed AllMetricsNames to AllInternalTimeseriesMetricsNames
to make clear what is being returned.

Demo:

killall -9 cockroach; rm -rf cockroach-data;
./cockroach start-single-node --insecure --background && \
./cockroach workload run kv --init \
    'postgres://[email protected]:26257?sslmode=disable' --duration=300s && \
./cockroach debug tsdump --format=raw --insecure > tsdump.gob && \
killall -9 cockroach && rm -rf cockroach-data && \
COCKROACH_DEBUG_TS_IMPORT_FILE=tsdump.gob ./cockroach start-single-node --insecure

image

Release justification: low-risk observability fix
Release note (ops change): The ./cockroach debug tsdump command now
downloads histogram timeseries it silently omitted previously.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg force-pushed the tsdump-all-metrics branch from ec3323d to 046b6fb Compare August 27, 2021 13:27
@tbg tbg changed the title ts: account for histograms in AllMetricsNames ts: include histogram quantiles in tsdump Aug 27, 2021
@tbg tbg requested a review from a team August 27, 2021 13:28
@tbg tbg marked this pull request as ready for review August 27, 2021 13:32
@tbg tbg force-pushed the tsdump-all-metrics branch from 046b6fb to 285d813 Compare August 27, 2021 13:41
@tbg tbg requested a review from a team August 27, 2021 13:42
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm: thx for all the docs in the code. Helped me understand why things have to be done the way they were.

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


pkg/server/status_test.go, line 1049 at r1 (raw file):

				// AllInternalTimeseriesMetricNames(). It's not a complete check but good enough in
				// practice. Ideally we wouldn't require `histogramMetricsNames` and
				// the associated manual step when adding a histogram. See:

nit: is this comment incomplete?

`cockroach debug tsdump` previously silently did not return metrics
backed by histograms. This is for technical reasons related to the
bookkeeping of metrics names and is rectified here by requiring some
extra tagging of metrics that are histograms so that they can be picked
up by tsdump. It's not pretty, but pragmatic: it works and it'll be
clear to anyone adding a histogram in the future how to proceed, even if
they may wonder why things work in such a roundabout manner (and if
they're curious about that, the relevant issues are linked in comments
as well).

I also renamed AllMetricsNames to AllInternalTimeseriesMetricsNames
to make clear what is being returned.

Demo:

```
killall -9 cockroach; rm -rf cockroach-data;
./cockroach start-single-node --insecure --background && \
./cockroach workload run kv --init \
    'postgres://[email protected]:26257?sslmode=disable' --duration=300s && \
./cockroach debug tsdump --format=raw --insecure > tsdump.gob && \
killall -9 cockroach && rm -rf cockroach-data && \
COCKROACH_DEBUG_TS_IMPORT_FILE=tsdump.gob ./cockroach start-single-node --insecure
```

![image](https://user-images.githubusercontent.com/5076964/131134624-b5471621-d23b-4ce7-9026-e8aeb3613231.png)

Release justification: low-risk observability fix
Release note (ops change): The ./cockroach debug tsdump command now
downloads histogram timeseries it silently omitted previously.
@tbg tbg force-pushed the tsdump-all-metrics branch from 285d813 to 4cbe4db Compare August 27, 2021 20:17
@tbg tbg requested a review from dhartunian August 27, 2021 20:17
Copy link
Member Author

@tbg tbg left a comment

Choose a reason for hiding this comment

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

TFTR! CI is red because of some LogicTest, so going to give it a shot

bors r=dhartunian

Dismissed @dhartunian from a discussion.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian)


pkg/server/status_test.go, line 1049 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: is this comment incomplete?

Sure is! Fixed.

@craig
Copy link
Contributor

craig bot commented Aug 27, 2021

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Aug 28, 2021

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