metrics: assign histogram metric type on histogram construction

[ericharmeling]

This commit assigns prometheusgo.MetricType_HISTOGRAM to the
Metadata.MetricType on histogram construction.

Before this change, GetMetadata() was returning the
Metadata.MetricType zero value (prometheusgo.MetricType_COUNTER)
for all histograms that did not explicitly specify the
prometheusgo.MetricType_HISTOGRAM for Metadata.MetricType in
their Metadata definitions. This prevented checks on histogram
Metadata.MetricType from properly evaluating the metrics as
histograms.

Fixes #106448.
Fixes #107701.

Releaes note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[ericharmeling]

For more details, see #107701 (comment)

[dhartunian]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @ericharmeling)

---

pkg/util/metric/metric.go line 404 at r1 (raw file):

func (h *Histogram) GetMetadata() Metadata {
	baseMetadata := h.Metadata
	baseMetadata.MetricType = prometheusgo.MetricType_HISTOGRAM

why isn't MetricType already set to prometheusgo.MetricType_HISTOGRAM upon metadata or histogram construction?

[ericharmeling]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @dhartunian)

---

pkg/util/metric/metric.go line 404 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

why isn't MetricType already set to prometheusgo.MetricType_HISTOGRAM upon metadata or histogram construction?

That's a good question.

We are only explicitly setting MetricType for a subset of metrics at metadata construction (only 3 histograms actually, in ttljob_metrics - see #107701 (comment)). For these metrics (most histograms), we are only ever returning the metric-specific MetricType via some getter function (either GetMetadata or GetType).

I have some guesses as to why this was designed this way, but I'm not 100% sure. So I opted to leave things the way they are, and just fix the metadata getter implementation. Do you think it's worth looking into and refactoring?

[abarganier]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian)

---

pkg/util/metric/metric.go line 404 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

That's a good question.

We are only explicitly setting MetricType for a subset of metrics at metadata construction (only 3 histograms actually, in ttljob_metrics - see #107701 (comment)). For these metrics (most histograms), we are only ever returning the metric-specific MetricType via some getter function (either GetMetadata or GetType).

I have some guesses as to why this was designed this way, but I'm not 100% sure. So I opted to leave things the way they are, and just fix the metadata getter implementation. Do you think it's worth looking into and refactoring?

I sort of agree with David that it feels a bit off to just set and return this in the GetMetadata() function, instead of setting it on construction.

We don't necessarily have to update the metric.Metadata for every histogram to do this. Can't we just add something to NewHistogram [1] to set it there?

The concern here is inconsistent state. When you call GetMetadata() on a Histogram, it tells you that the MetricType is HISTOGRAM. But other code paths that do any sort of introspection on a Histogram won't see this same state for MetricType. That can lead to nasty bugs in the future because people wouldn't expect that it's only artificially set when GetMetadata() is called, but it's not actually reflected in the underlying state.

I think unless there's a good reason to avoid setting it in NewHistogram, we should just do it there for consistency.

[ericharmeling]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian)

---

pkg/util/metric/metric.go line 404 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

I sort of agree with David that it feels a bit off to just set and return this in the GetMetadata() function, instead of setting it on construction.

We don't necessarily have to update the metric.Metadata for every histogram to do this. Can't we just add something to NewHistogram [1] to set it there?

The concern here is inconsistent state. When you call GetMetadata() on a Histogram, it tells you that the MetricType is HISTOGRAM. But other code paths that do any sort of introspection on a Histogram won't see this same state for MetricType. That can lead to nasty bugs in the future because people wouldn't expect that it's only artificially set when GetMetadata() is called, but it's not actually reflected in the underlying state.

I think unless there's a good reason to avoid setting it in NewHistogram, we should just do it there for consistency.

That makes a lot of sense! Updated this PR to assign the right metric type on histogram construction.

[abarganier]

Changes looking good!

Should we also do this in the runtimeHistogram?

cockroach/pkg/util/schedulerlatency/histogram.go

Line 48 in b5bac44

func newRuntimeHistogram(metadata metric.Metadata, buckets []float64) *runtimeHistogram {

Seems like it's off in its own world, but it uses the same Metadata type 🤷

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian)

[ericharmeling]

TFTR!

Should we also need to do this in the runtimeHistogram?

Good catch - I think we should do it while were doing this. 🤷‍♂️

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian)

[dhartunian]

maybe later on if we touch this code again it might be worth creating separate Metadata structs for different types. There's no reason to mix up counter Metadata with a histogram Metadata.

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier)

[abarganier]

- nice work Eric!

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ericharmeling)

[abarganier]

- nice work Eric!

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ericharmeling)

[ericharmeling]

TFTRs!

maybe later on if we touch this code again it might be worth creating separate Metadata structs for different types. There's no reason to mix up counter Metadata with a histogram Metadata.

Right on. Should I make an issue for this work?

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ericharmeling)

[ericharmeling]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from fb8f99b to blathers/backport-release-22.2-108597: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}