Metric metadata endpoint

[sploiselle]

This PR integrates two changes:

• Makes explicit more metric metadata properties
• Adds an endpoint to make all metric metadata externally consumbale

Explicit Metric Metadata

Timeseries metrics had two implicit metadata properties:

• Units, which is the unit of data collected (Count, Bytes, or Duration)
• AxisLabel, which is the element measured by the metric (i.e. what the Y-axis represents)

Because these properties are used to display charts to users and do not change for the lifetime of
the metric, they should be defined when the when the metric is created. This ensures end users can
consistently and meaningfully interpret data in the way the metric's author intended.

Testing

This PR adds a test to status_test.go to ensure that all metrics metadata have a Name, Help, and AxisLabel defined. Because Units have a zero value of "Count", the test doesn't check for it being set.

Metric Metadata Endpoint

This PR adds an endpoint at <Admin UI>/_admin/metricmetadata that exposes all metric metadata, including the new Units and AxisLabel properties added in the first commit.

While this endpoint doesn't have much intrinsic value, it will be leveraged to generate a catalog of timeseries charts. That commit is ready to go, but is large, so wanted to make more incremental changes by splitting them into two PRs.

[cockroach-teamcity]

This change is 

[tbg]

Haven't reviewed in detail, but I was just wanting this. Thanks for reading my mind!

[tbg]

Reviewed 22 of 22 files at r1, 6 of 6 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

pkg/kv/txn_coord_sender.go, line 167 at r1 (raw file):

		Name:      "txn.aborts",
		Help:      "Number of aborted KV transactions",
		Units:     metric.UnitsCount,

nit: s/Units/Unit/ everywhere.

---

pkg/server/status_test.go, line 416 at r3 (raw file):

// TestMetricsMetadata ensures that each metric has a Name, Help, and
// AxisLabel. Because Metadata.Units has a zero value (of "Count"),

You should be able to not use the zero value for that.

---

pkg/server/serverpb/admin.proto, line 564 at r2 (raw file):

// REST-style HTTP endpoints that locally proxy to the gRPC endpoints.
service Admin {

Spurious whitespace.

---

pkg/server/serverpb/admin.proto, line 742 at r2 (raw file):

    };
  }
  

Spurious space.

---

pkg/server/status/recorder.go, line 325 at r2 (raw file):

	mr.mu.nodeRegistry.GetMetricsMetadata(metrics)

	// Get a random storeID

..

---

pkg/server/status/recorder.go, line 333 at r2 (raw file):

	}

	//Get metric metadata from that store because all stores have the same metadata

nit: trailing dot and leading space.

---

pkg/storage/metrics.go, line 369 at r1 (raw file):

		Help:      "Number of disk reads per query",
		Units:     metric.UnitsCount,
		AxisLabel: metric.AxisDiskReadsperQuery,

not Per?

---

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

// each metric object.
type Metadata struct {
	Name, Help, AxisLabel, MetricType string

AxisLabel and MetricType should be enums and since go doesn't have that, integers. Take a look here for inspiration.

---

pkg/util/metric/registry.go, line 113 at r2 (raw file):

}

// GetMetricsMetadata gets metadata from all tracked metrics

..

---

Comments from Reviewable

[couchand]

Overall I think this is a great direction to go in. I left a bunch of comments about the implementation and some about specifics on the updates to existing metadata.

Also, I have two general thoughts:

I think most of the usages of metric.AxisTime and metric.AxisSize would be improved by using more specific labels.

Many of the metrics listed as a metric.UnitCount are actually rates, and it may be a good idea to encode that information. Granted, it does make the unit description more complicated. It's more complicated by the fact that sometimes (always??) the rate is calculated by the query, not in the metric as stored.

---

Reviewed 22 of 22 files at r1, 6 of 6 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

pkg/rpc/clock_offset.go, line 51 at r3 (raw file):

		Help:      "Mean clock offset with other nodes in nanoseconds",
		Units:     metric.UnitsDuration,
		AxisLabel: metric.AxisTime,

These three metrics could use more specific labels.

---

pkg/security/certificate_manager.go, line 36 at r3 (raw file):

		Name:      "security.certificate.expiration.ca",
		Help:      "Expiration timestamp in seconds since Unix epoch for the CA certificate. 0 means no certificate or error.",
		Units:     metric.UnitsDuration,

These are actually not durations, they are a point in time. They could also use more specific labels.

---

pkg/server/admin.go, line 152 at r3 (raw file):

}

// AllMetricMetadata returns all metric's metadata.

s/metric's/metrics'/

---

pkg/server/node.go, line 69 at r3 (raw file):

		Help:      "Latency in nanoseconds of batch KV requests executed on this node",
		Units:     metric.UnitsDuration,
		AxisLabel: metric.AxisTime,

This could use a more specific label.

---

pkg/server/status_test.go, line 427 at r3 (raw file):

	for _, v := range metricsMetadata {
		if v.Name == "" {
			t.Fatal("metric missing name")

It would be helpful to have something to point to which metric, but maybe there isn't anything.

---

pkg/server/serverpb/admin.proto, line 565 at r3 (raw file):

service Admin {

Please remove these empty lines.

---

pkg/server/serverpb/admin.proto, line 742 at r3 (raw file):

    };
  }
  

Please remove the spaces on this empty line.

---

pkg/server/status/recorder.go, line 178 at r3 (raw file):

		Name:      "node-id",
		Help:      "node ID with labels for advertised RPC and HTTP addresses",
		Units:     metric.UnitsCount,

I don't think this is a count.

---

pkg/server/status/recorder.go, line 308 at r3 (raw file):

// GetMetricsMetadata returns the metadata from all metrics tracked in the node's
// nodeRegistry and a randomly selected storeRegistry

Our style is to use full sentences with a period in comments.

---

pkg/server/status/recorder.go, line 318 at r3 (raw file):

			log.Warning(context.TODO(), "MetricsRecorder.GetMetricsMetadata() called before NodeID allocation")
		}
		return nil

Maybe worth returning an error here instead?

---

pkg/server/status/runtime.go, line 58 at r3 (raw file):

	}
	metaGCPausePercent = metric.Metadata{
		Name: "sys.gc.pause.percent", Help: "Current GC pause percentage", Units: metric.UnitsCount, AxisLabel: metric.AxisPercent,

This and the below percentages aren't counts.

---

pkg/server/status/runtime.go, line 82 at r3 (raw file):

	}
	metaUptime = metric.Metadata{
		Name: "sys.uptime", Help: "Process uptime in seconds", Units: metric.UnitsDuration, AxisLabel: metric.AxisTime,

The units here don't match with the comment on the enum value, which will be more obvious if the units are all real units.

---

pkg/server/status/runtime.go, line 148 at r3 (raw file):

	// Build information.
	metaBuildTimestamp := metric.Metadata{
		Name: "build.timestamp", Help: "Build information", Units: metric.UnitsDuration, AxisLabel: metric.AxisTimeSinceUnixEpoch,

Not a duration but rather a point in time.

---

pkg/storage/metrics.go, line 90 at r3 (raw file):

		Help:      "Largest number of intervals in any CommandQueue's interval tree",
		Units:     metric.UnitsCount,
		AxisLabel: metric.AxisCommands,

I don't think this is measuring commands.

---

pkg/storage/metrics.go, line 222 at r3 (raw file):

		Help:      "Count of all values",
		Units:     metric.UnitsCount,
		AxisLabel: metric.AxisKeys,

This seems to be measuring values, not keys. I'm not at all clear on how they could be different. Perhaps MVCC comes into play?

---

pkg/storage/metrics.go, line 233 at r3 (raw file):

		Name:      "intentage",
		Help:      "Cumulative age of intents in seconds",
		Units:     metric.UnitsDuration,

Likewise, the units on this and the next metric are incompatible with the enum comment.

---

pkg/storage/metrics.go, line 245 at r3 (raw file):

		Name:      "lastupdatenanos",
		Help:      "Time in nanoseconds since Unix epoch at which bytes/keys/intents metrics were last updated",
		Units:     metric.UnitsDuration,

This is a point in time, not a duration.

---

pkg/storage/metrics.go, line 451 at r3 (raw file):

		Help:      "Latency histogram in nanoseconds for committing Raft log entries",
		Units:     metric.UnitsCount,
		AxisLabel: metric.AxisTime,

This label seems incongruous with the units...

---

pkg/storage/metrics.go, line 813 at r3 (raw file):

		Help:      "Number of transactions present in the AbortSpan scanned from the engine",
		Units:     metric.UnitsCount,
		AxisLabel: metric.AxisEntries,

Looks to be a count of transactions.

---

pkg/ts/tspb/timeseries.proto, line 131 at r1 (raw file):

}

// MetricAxisUnits described the unit of data collected.

Axis is a property of display not of the metric itself, so I don't think the term should appear anywhere in this PR.

---

pkg/ts/tspb/timeseries.proto, line 133 at r1 (raw file):

// MetricAxisUnits described the unit of data collected.
enum MetricAxisUnits {
  // Units are a simple count.

s/simple/unitless/

---

pkg/ts/tspb/timeseries.proto, line 138 at r1 (raw file):

  Bytes = 1;
  // Units are durations expressed in nanoseconds.
  Duration = 2;

This enum mixes units (bytes) with measures (duration). I would suggest that we stick to units, since that's more specific and relevant for this usage. So if duration metrics are measured in nanoseconds, this enum value should be Nanoseconds.

---

pkg/ts/tspb/timeseries.proto, line 146 at r1 (raw file):

  required string help = 2 [(gogoproto.nullable) = false];
  required MetricAxisUnits units = 3 [(gogoproto.nullable) = false];
  required string axisLabel = 4 [(gogoproto.nullable) = false];

Likewise, I don't think "axis" is appropriate here.

---

pkg/ts/tspb/timeseries.proto, line 147 at r1 (raw file):

  required MetricAxisUnits units = 3 [(gogoproto.nullable) = false];
  required string axisLabel = 4 [(gogoproto.nullable) = false];
  required string metricType = 5 [(gogoproto.nullable) = false];

From usage this seems to be encoding the same information as the prometheus metric type. What's the intended usage of this? It seems to deviate from the pattern of the rest of the metadata.

---

pkg/ts/tspb/timeseries.proto, line 135 at r3 (raw file):

  // Units are a simple count.
  Count = 0;
  // Units are a count of bytes.

Why are all these comments just "Units..."?

---

pkg/ui/src/views/shared/components/metricQuery/index.tsx, line 28 at r3 (raw file):

import TimeSeriesQueryAggregator = protos.cockroach.ts.tspb.TimeSeriesQueryAggregator;
import TimeSeriesQueryDerivative = protos.cockroach.ts.tspb.TimeSeriesQueryDerivative;
export import AxisUnits = protos.cockroach.ts.tspb.MetricAxisUnits;

I think this will need to be a manual mapping to account for the technical debt of the previous code.

---

pkg/util/metric/counter_with_rates.go, line 58 at r3 (raw file):

		es[scale] = NewRate(scale.d)
	}
	c := NewCounter(metadata)

Is this change safe? If there's not a failing test from it I suppose it's likely fine, but I wonder what the purpose of the old code was.

---

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

	"time"

	"github.com/cockroachdb/cockroach/pkg/ts/tspb"

It seems odd to have the dependency arrow in this direction. I think it would be preferable to not have code in pkg/util depend on crdb code outside of util.

---

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

	GetUnits() tspb.MetricAxisUnits
	// GetAxisLabel is a method on Metadata
	GetAxisLabel() string

Are these methods on PrometheusExportable used anywhere?

---

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

)

// AxisLabel constants for metric.Metadata.AxisLabel

I'm not sure it's adding much value listing all the labels here. These are a wide and growing set, putting the value inline with the title & description seems more appropriate.

---

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

}

// GetAxisLabel returns the metric's aix labek

sp?

---

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

// GetMetadata returns all of the metric's metadata
func (m *Metadata) GetMetadata() tspb.MetricMetadata {

It seems very odd to me to have a method named GetMetadata on a struct named Metadata.

---

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

// precision.
func NewHistogram(metadata Metadata, duration time.Duration, maxVal int64, sigFigs int) *Histogram {
	metadata.MetricType = "Histogram"

I'm pretty uncomfortable with these argument mutations.

---

pkg/util/metric/registry.go, line 116 at r3 (raw file):

func (r *Registry) GetMetricsMetadata(dest map[string]tspb.MetricMetadata) {
	for _, v := range r.tracked {
		dest[v.GetName()] = v.GetMetadata()

I'm pretty uncomfortable with this argument mutation.

---

Comments from Reviewable

[mrtracy]

Review status: all files reviewed at latest revision, 43 unresolved discussions, some commit checks failed.

---

pkg/server/status/recorder.go, line 323 at r3 (raw file):

	metrics := make(map[string]tspb.MetricMetadata, 0)

	mr.mu.nodeRegistry.GetMetricsMetadata(metrics)

(This is about removing the dependency from util/metrics -> tspb; see comments below for details).

Instead of calling GetMetricsMetadata(), use registry.Each() here; you can see an example of its usage in this very file. You'll also need to define an interface with the GetMetadata method. Example:

type MetadataSource interface {
  GetMetadata() metric.Metadata
}

func metadataToProto(meta metrics.Metadata) tspb.MetricsMetadata {
  return tspb.MetricsMetadata{
		Name:       m.Name,
		Help:       m.Help,
		Units:      m.Units,
		AxisLabel:  m.AxisLabel,
		MetricType: m.MetricType,
  }
}

mr.mu.nodeRegistry.Each(func (name str, mtr interface{}) {
  if metadataSrc, ok := mtr.(MetadataSource); ok {
    metrics[name] = metadataToProto(metadataSrc.GetMetadata())
  }
})

---

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

Previously, couchand (Andrew Couch) wrote…

It seems odd to have the dependency arrow in this direction. I think it would be preferable to not have code in pkg/util depend on crdb code outside of util.

I'm going to agree with this; the util collection currently seems to have no obvious dependencies on cockroach code, so let's try to keep it that way. I've outlined a few strategies for doing this in my comments below.

---

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

Previously, tschottdorf (Tobias Schottdorf) wrote…

AxisLabel and MetricType should be enums and since go doesn't have that, integers. Take a look here for inspiration.

MetricType yes, AxisLabel no - AxisLabel should remain a string and be inlined like Name and Help (see Andrew's comment below).

MetricAxisUnits should also be defined here as a separate enumeration in order to break the util -> tspb dependency.

---

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

Previously, couchand (Andrew Couch) wrote…

I'm not sure it's adding much value listing all the labels here. These are a wide and growing set, putting the value inline with the title & description seems more appropriate.

Agreed, these should be inlined; while there is some repetition, it's not enough in my opinion to justify moving to constant.

---

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

Previously, couchand (Andrew Couch) wrote…

It seems very odd to me to have a method named GetMetadata on a struct named Metadata.

This should also be removed as part of breaking the util -> tspb dependency.

---

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

Previously, couchand (Andrew Couch) wrote…

I'm pretty uncomfortable with these argument mutations.

I agree in this case, it's not at all clear that the metadata parameter will be mutated. Instead, I would suggest adding an explicit GetMetadata() method to each metric type that fills in the type without mutating. For example:

func (h *Histogram) GetMetadata() Metadata {
  baseMetadata := h.Metadata
  baseMetadata.MetricType = "Histogram"
  return baseMetadata
}

This has no mutations as Metadata is a value type.

Notice that, as per both mine and Andrew's comments above about removing the dependency util/metric -> tspb, this returns a Metadata instead of a tspb.MetricMetadata. You'll need to convert it to tspb.MetricMetadata in the server/status package.

---

pkg/util/metric/registry.go, line 116 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I'm pretty uncomfortable with this argument mutation.

I think the mutation itself is fine in this case, the argument name "dest" is clear that the parameter is being mutated. That said, naming this method something like "WriteMetricsMetadata" would be more appropriate; essentially, using a verb that implies modification.

However, see both mine and Andrew's comments above about removing the dependency util/metric -> tspb; instead of this method, you'll want to use Each() and convert to tspb.MetricMetadata in the higher level package.

---

Comments from Reviewable

[sploiselle]

@mrtracy @couchand Do either of you have opinions about breaking the dependency I've created by creating a metric.proto file? I had originally placed my proto modifications in the tspb package for the sake of using existing surface area without the foresight that would create circular dependencies.

This new proto could define the entire Metadata struct, which I think is beneficial because it:

• Removes the circular dependency
• Represents a cleaner abstraction (metric.Metadata vs tspb.MetricMetadata) and leverages an existing struct
• Presents a clean solution to removing the duplicative definition of the Prometheus metric type (comment here)

I've already done the work to ensure we can import the Prometheus field, so wanted to check the viability of the solution before moving forward.

[sploiselle]

Review status: 1 of 33 files reviewed at latest revision, 44 unresolved discussions, all commit checks successful.

---

pkg/kv/txn_coord_sender.go, line 167 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

nit: s/Units/Unit/ everywhere.

Done.

---

pkg/rpc/clock_offset.go, line 51 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

These three metrics could use more specific labels.

Done.

---

pkg/security/certificate_manager.go, line 36 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

These are actually not durations, they are a point in time. They could also use more specific labels.

Done.

---

pkg/server/admin.go, line 152 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

s/metric's/metrics'/

Done.

---

pkg/server/node.go, line 69 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

This could use a more specific label.

Done.

---

pkg/server/status_test.go, line 416 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You should be able to not use the zero value for that.

Done.

---

pkg/server/status_test.go, line 427 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

It would be helpful to have something to point to which metric, but maybe there isn't anything.

Without a name, there's nothing to work with. I added this mainly as a formality. Because that's the case, do you think it's worth including at all?

---

pkg/server/serverpb/admin.proto, line 564 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Spurious whitespace.

Done.

---

pkg/server/serverpb/admin.proto, line 742 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Spurious space.

Done.

---

pkg/server/serverpb/admin.proto, line 565 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Please remove these empty lines.

Done.

---

pkg/server/serverpb/admin.proto, line 742 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Please remove the spaces on this empty line.

Done.

---

pkg/server/status/recorder.go, line 325 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

..

Done.

---

pkg/server/status/recorder.go, line 333 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

nit: trailing dot and leading space.

Done.

---

pkg/server/status/recorder.go, line 178 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I don't think this is a count.

Added Const type

---

pkg/server/status/recorder.go, line 308 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Our style is to use full sentences with a period in comments.

Done.

---

pkg/server/status/recorder.go, line 318 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Maybe worth returning an error here instead?

I duplicated this code from a few lines above it. Glad to change it, but let me know if I should change it in both places.

---

pkg/server/status/recorder.go, line 323 at r3 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

(This is about removing the dependency from util/metrics -> tspb; see comments below for details).

Instead of calling GetMetricsMetadata(), use registry.Each() here; you can see an example of its usage in this very file. You'll also need to define an interface with the GetMetadata method. Example:

type MetadataSource interface {
  GetMetadata() metric.Metadata
}

func metadataToProto(meta metrics.Metadata) tspb.MetricsMetadata {
  return tspb.MetricsMetadata{
		Name:       m.Name,
		Help:       m.Help,
		Units:      m.Units,
		AxisLabel:  m.AxisLabel,
		MetricType: m.MetricType,
  }
}

mr.mu.nodeRegistry.Each(func (name str, mtr interface{}) {
  if metadataSrc, ok := mtr.(MetadataSource); ok {
    metrics[name] = metadataToProto(metadataSrc.GetMetadata())
  }
})

I opted to solve this by creating util/metrics/metric.proto, which I think is a cleaner solution. Let me know if there are problems with that approach.

---

pkg/server/status/runtime.go, line 58 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

This and the below percentages aren't counts.

Done.

---

pkg/server/status/runtime.go, line 82 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

The units here don't match with the comment on the enum value, which will be more obvious if the units are all real units.

Done.

---

pkg/server/status/runtime.go, line 148 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Not a duration but rather a point in time.

Done.

---

pkg/storage/metrics.go, line 369 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

not Per?

Done.

---

pkg/storage/metrics.go, line 90 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I don't think this is measuring commands.

Done.

---

pkg/storage/metrics.go, line 222 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

This seems to be measuring values, not keys. I'm not at all clear on how they could be different. Perhaps MVCC comes into play?

Done.

---

pkg/storage/metrics.go, line 233 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Likewise, the units on this and the next metric are incompatible with the enum comment.

Done.

---

pkg/storage/metrics.go, line 245 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

This is a point in time, not a duration.

Done.

---

pkg/storage/metrics.go, line 451 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

This label seems incongruous with the units...

Done.

---

pkg/storage/metrics.go, line 813 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Looks to be a count of transactions.

Done.

---

pkg/util/metric/counter_with_rates.go, line 58 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Is this change safe? If there's not a failing test from it I suppose it's likely fine, but I wonder what the purpose of the old code was.

Talked to @tschottdorf who was on the blame for this and he said this change was fine, given passing tests.

---

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

Previously, mrtracy (Matt Tracy) wrote…

I'm going to agree with this; the util collection currently seems to have no obvious dependencies on cockroach code, so let's try to keep it that way. I've outlined a few strategies for doing this in my comments below.

Removed the bad dependency cycle by creating util/metrics/metric.proto.

---

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

Previously, couchand (Andrew Couch) wrote…

Are these methods on PrometheusExportable used anywhere?

Removed

---

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

Previously, mrtracy (Matt Tracy) wrote…

MetricType yes, AxisLabel no - AxisLabel should remain a string and be inlined like Name and Help (see Andrew's comment below).

MetricAxisUnits should also be defined here as a separate enumeration in order to break the util -> tspb dependency.

Done.

---

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

Previously, mrtracy (Matt Tracy) wrote…

Agreed, these should be inlined; while there is some repetition, it's not enough in my opinion to justify moving to constant.

Done.

---

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

Previously, couchand (Andrew Couch) wrote…

sp?

Done.

---

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

Previously, mrtracy (Matt Tracy) wrote…

This should also be removed as part of breaking the util -> tspb dependency.

Done.

---

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

Previously, mrtracy (Matt Tracy) wrote…

I agree in this case, it's not at all clear that the metadata parameter will be mutated. Instead, I would suggest adding an explicit GetMetadata() method to each metric type that fills in the type without mutating. For example:

func (h *Histogram) GetMetadata() Metadata {
  baseMetadata := h.Metadata
  baseMetadata.MetricType = "Histogram"
  return baseMetadata
}

This has no mutations as Metadata is a value type.

Notice that, as per both mine and Andrew's comments above about removing the dependency util/metric -> tspb, this returns a Metadata instead of a tspb.MetricMetadata. You'll need to convert it to tspb.MetricMetadata in the server/status package.

Done.

---

pkg/util/metric/registry.go, line 113 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

..

Done.

---

pkg/util/metric/registry.go, line 116 at r3 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

I think the mutation itself is fine in this case, the argument name "dest" is clear that the parameter is being mutated. That said, naming this method something like "WriteMetricsMetadata" would be more appropriate; essentially, using a verb that implies modification.

However, see both mine and Andrew's comments above about removing the dependency util/metric -> tspb; instead of this method, you'll want to use Each() and convert to tspb.MetricMetadata in the higher level package.

Changed name, but opted for different approach for breaking util/metric -> tspb dependency. @mrtracy Let me know if there are other considerations I need to make here.

---

pkg/ts/tspb/timeseries.proto, line 131 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

Axis is a property of display not of the metric itself, so I don't think the term should appear anywhere in this PR.

Done.

---

pkg/ts/tspb/timeseries.proto, line 133 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

s/simple/unitless/

Done.

---

pkg/ts/tspb/timeseries.proto, line 138 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

This enum mixes units (bytes) with measures (duration). I would suggest that we stick to units, since that's more specific and relevant for this usage. So if duration metrics are measured in nanoseconds, this enum value should be Nanoseconds.

Done.

---

pkg/ts/tspb/timeseries.proto, line 146 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

Likewise, I don't think "axis" is appropriate here.

Done.

---

pkg/ts/tspb/timeseries.proto, line 147 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

From usage this seems to be encoding the same information as the prometheus metric type. What's the intended usage of this? It seems to deviate from the pattern of the rest of the metadata.

De-duplicated by importing Prometheus MetricType into metric.proto

---

pkg/ts/tspb/timeseries.proto, line 135 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Why are all these comments just "Units..."?

Fixed this everywhere. I just inherited it from the existing code, but have improved the description in metric.proto.

---

pkg/ui/src/views/shared/components/metricQuery/index.tsx, line 28 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I think this will need to be a manual mapping to account for the technical debt of the previous code.

Removed these changes and will address them in a future PR when this information actually integrates with the custom chart tool.

---

Comments from Reviewable

[sploiselle]

Integrated all feedback. This reflects two substantial changes.

metric.proto

metric.Metadata is now defined in metric.proto, which has the benefits outlined in my last comment but will quickly summarize here:

• Removed the tspb->util dependency that originally existed.
• Simplified use of the Metadata struct by placing it in the package where it's most often used.

Importing Prometheus's client_model proto

In support of metric.proto, I worked with @benesch to include github.com/prometheus/client_model/metrics.proto. This let us embed Prometheus fields directly into the struct, which resolved an outstanding question about duplicating the Prometheus metric types.

• This necessitated changes to the Makefile and Gopkg.toml, which Nikhil helped me write.
• The Prometheus proto file's LabelPair field doesn't work with gogoproto.marhsaler and .unmarshaler, both which became required for gRPC in a recent version bump (AFAICT; it worked before the GRPC, and no longer works after it).
  I resolved this issue by creating and using metric.LabelPair, which is trivially convertible to the Prometheus LabelPair field. Chatted with Nikhil and demoed the issue for him, and he agreed this was a reasonable solution.
  I believe this solution also represents a better tradeoff than re-declaring the Promethus metric types, which are an enum, and is therefore slightly more fragile to ensure consistency with.

[mrtracy]

It looks like you upgraded yarn.lock, even though you didn't touch any ui components in this commit. That should be reverted.

Otherwise, this looks to be in pretty good shape. I had some questions about the "Key" fields on the response, if you could clarify their intended usage I would appreciate it!

---

Review status: 1 of 33 files reviewed at latest revision, 44 unresolved discussions, some commit checks failed.

---

pkg/server/status_test.go, line 424 at r7 (raw file):

	metricsMetadata := s.recorder.GetMetricsMetadata()

	for _, v := range metricsMetadata {

Add a sanity check here that there are more than 0 metricsMetadata objects returned. This will pass trivially if GetMetricsMetadata returns nothing.

Might even be safe to assert on the exact number; it would need to be updated each time somebody adds a metric, but that's good confirmation that the metric was successfully added.

---

pkg/server/serverpb/admin.proto, line 561 at r7 (raw file):

  // following gogoprotobuf extensions that we enable by default, so turn them off.
  
  map<int32, string> displayUnitKey = 1 [(gogoproto.nullable) = false];

What is the purpose of these two "Key" maps? I'm not sure how they're intended to be used, especially since they just appear to be copying the enumeration maps from metrics.proto. Where are you intending to use the string values for these units?

---

pkg/util/metric/metric.proto, line 62 at r7 (raw file):

  optional io.prometheus.client.MetricType metricType = 5 [(gogoproto.nullable) = false];
  repeated LabelPair labels = 6;
}

needs a newline at EOF

---

Comments from Reviewable

[sploiselle]

Review status: 1 of 32 files reviewed at latest revision, 47 unresolved discussions, all commit checks successful.

---

pkg/server/status_test.go, line 424 at r7 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Add a sanity check here that there are more than 0 metricsMetadata objects returned. This will pass trivially if GetMetricsMetadata returns nothing.

Might even be safe to assert on the exact number; it would need to be updated each time somebody adds a metric, but that's good confirmation that the metric was successfully added.

Never would have thought of that! Thank you.

My apprehension with creating a const for the value is that:

• It would always have to be incremented by hand and could be susceptible from multiple PRs adding metrics at the same time.
• Not all servers pull back the same metrics; e.g. if they don't have certs, this endpoint won't pull back the metadata related to cert expirations. Those calculations and permutations seem like weedy problem.

However, if there's another solution or you think enumerating the number of metrics is worth doing, let me know and I can figure something out.

---

pkg/server/serverpb/admin.proto, line 561 at r7 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

What is the purpose of these two "Key" maps? I'm not sure how they're intended to be used, especially since they just appear to be copying the enumeration maps from metrics.proto. Where are you intending to use the string values for these units?

Derp. Definitely deserves comments.

#25765 is a feature request to make generating metric documentation simpler, which we'll be able to complete by using this endpoint. By including these key maps, we'll be able to more trivially/reliably create documentation from this endpoint, even if the enum grows/changes.

---

pkg/util/metric/metric.proto, line 62 at r7 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

needs a newline at EOF

Done.

---

Comments from Reviewable

[mrtracy]

I'll preemptively give this an LGTM assuming the sanity check is added to that test, everything else looks good so far as I can tell from a code review.

---

Review status: 1 of 32 files reviewed at latest revision, 47 unresolved discussions, some commit checks failed.

---

pkg/server/status_test.go, line 424 at r7 (raw file):

Previously, sploiselle (Sean Loiselle) wrote…

Never would have thought of that! Thank you.

My apprehension with creating a const for the value is that:

• It would always have to be incremented by hand and could be susceptible from multiple PRs adding metrics at the same time.
• Not all servers pull back the same metrics; e.g. if they don't have certs, this endpoint won't pull back the metadata related to cert expirations. Those calculations and permutations seem like weedy problem.

However, if there's another solution or you think enumerating the number of metrics is worth doing, let me know and I can figure something out.

Hmm, if the number of metrics isn't constant, than just do a sanity check - just set the number to something like 200 (there should be at least that many), just to make sure the number isn't zero.

---

pkg/server/serverpb/admin.proto, line 561 at r7 (raw file):

Previously, sploiselle (Sean Loiselle) wrote…

Derp. Definitely deserves comments.

#25765 is a feature request to make generating metric documentation simpler, which we'll be able to complete by using this endpoint. By including these key maps, we'll be able to more trivially/reliably create documentation from this endpoint, even if the enum grows/changes.

Gotcha.

---

Comments from Reviewable

[couchand]

Reviewed 3 of 34 files at r4, 3 of 9 files at r5, 16 of 26 files at r7, 3 of 5 files at r8, 7 of 7 files at r9, 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

---

pkg/rpc/clock_offset.go, line 62 at r10 (raw file):

		Name:        "round-trip-latency",
		Help:        "Distribution of round-trip latencies with other nodes in nanoseconds",
		Unit:        "Roundtrip Latencies",

s/Latencies/Latency/

---

pkg/server/status_test.go, line 427 at r3 (raw file):

Previously, sploiselle (Sean Loiselle) wrote…

Without a name, there's nothing to work with. I added this mainly as a formality. Because that's the case, do you think it's worth including at all?

this is probably about as good as we can get...

---

pkg/server/serverpb/admin.proto, line 561 at r7 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Gotcha.

Maybe the API endpoint should just return the stringified form? That would be more useful than forcing the user to do the mapping.

---

pkg/server/status/runtime.go, line 82 at r3 (raw file):

Previously, sploiselle (Sean Loiselle) wrote…

Done.

umm... I don't think that solved the problem: there's still a mismatch.

---

pkg/storage/metrics.go, line 233 at r3 (raw file):

Previously, sploiselle (Sean Loiselle) wrote…

Done.

It's still seconds vs nanoseconds...

---

pkg/storage/metrics.go, line 738 at r10 (raw file):

		Name:        "queue.split.processingnanos",
		Help:        "Nanoseconds spent processing replicas in the split queue",
		Unit:        "CPU Time",

All the other ones are "Processing Time", why is this one different?

---

pkg/util/metric/metric.proto, line 48 at r10 (raw file):

  // Percent expresses that the metric represents a percentage value.
  PERCENT = 5;
  // Timestamp expresses that the metric represents time since the Unix epoch.

I'd at least comment here and maybe add to the enum name the units of time since the epoch, since both seconds and milliseconds are common.

---

pkg/util/metric/metric.proto, line 59 at r10 (raw file):

  required string help = 2 [(gogoproto.nullable) = false];
  required string unit = 3 [(gogoproto.nullable) = false];
  required DisplayUnit displayUnit = 4 [(gogoproto.nullable) = false];

I'm not sure it's very clear what the difference between unit and displayUnit is. Can we come up with clearer names?

---

Comments from Reviewable

[sploiselle]

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/rpc/clock_offset.go, line 62 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

s/Latencies/Latency/

Done.

---

pkg/server/status_test.go, line 424 at r7 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Hmm, if the number of metrics isn't constant, than just do a sanity check - just set the number to something like 200 (there should be at least that many), just to make sure the number isn't zero.

Added a check to ensure that there are at least 200 metrics returned.

---

pkg/server/serverpb/admin.proto, line 561 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

Maybe the API endpoint should just return the stringified form? That would be more useful than forcing the user to do the mapping.

The primary use case is still to power the chart catalog, in which I was planning to use the actual enum value. I could convert them back from strings to the correct enum, but that feels like putting complexity in the wrong place.

FWIW, I'm planning to write the tool to turn these into docs, and am glad to be the user handling the complexity.

LMK if you'd prefer another approach like including both the enum and the string value or something.

---

pkg/server/status/runtime.go, line 82 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

umm... I don't think that solved the problem: there's still a mismatch.

Derp. Missed the Help text on the original pass. Added a SECONDS enum and found the metrics that mention that they're currently collected in seconds and have corrected their units.

I also removed mention of the units from the help text because they were used inconsistently when the units were nanoseconds, and they served more of a function historically when the unit wasn't explicitly set when the metadata was created. With an explicit statement of the metric's units, it's duplicative.

---

pkg/storage/metrics.go, line 233 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

It's still seconds vs nanoseconds...

Same thing here with adding seconds and removing mention of units from help, as well as fixing the Unit.

---

pkg/storage/metrics.go, line 738 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

All the other ones are "Processing Time", why is this one different?

Oversight. Fixed.

---

pkg/util/metric/metric.proto, line 48 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

I'd at least comment here and maybe add to the enum name the units of time since the epoch, since both seconds and milliseconds are common.

Good catch here. There are timestamps in both seconds and nanoseconds, so that's fixed by adding TIMESTAMP_SEC. I didn't find any timestamps that refer to milliseconds, but think I addressed the spirit of this issue.

I also filed an issue about second-based metrics (incl. timestamps) not rendering correctly in charts with the Duration unit.

---

pkg/util/metric/metric.proto, line 59 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

I'm not sure it's very clear what the difference between unit and displayUnit is. Can we come up with clearer names?

How about "Measurement" and "Unit"? I tired Label, but that felt like it created an ambiguity with GetLabel() and GetLabels().

I think this works well because you could say, "I measured [measurement] in/as a [unit]", e.g. "I measured age in seconds." or "I measured MVCC Values as a count."

---

Comments from Reviewable

[couchand]

I think my main concern is about throwing in those static string maps. They don't seem to be adding specific value but they are introducing an overhead in the service definition.

Otherwise, just left a bunch of lints on comments. Thanks!

---

Reviewed 1 of 34 files at r4, 1 of 26 files at r7, 1 of 5 files at r8, 25 of 26 files at r11, 1 of 1 files at r12.
Review status: complete! 0 of 0 LGTMs obtained

---

pkg/rpc/clock_offset.go, line 55 at r12 (raw file):

	metaClockOffsetStdDevNanos = metric.Metadata{
		Name:        "clock-offset.stddevnanos",
		Help:        "Stdddev clock offset with other nodes",

not your change but there's a typo here

---

pkg/server/serverpb/admin.proto, line 561 at r7 (raw file):

Previously, sploiselle (Sean Loiselle) wrote…

The primary use case is still to power the chart catalog, in which I was planning to use the actual enum value. I could convert them back from strings to the correct enum, but that feels like putting complexity in the wrong place.

FWIW, I'm planning to write the tool to turn these into docs, and am glad to be the user handling the complexity.

LMK if you'd prefer another approach like including both the enum and the string value or something.

We shouldn't be creating this explicitly, since the protobuf toolchain can already provide this functionality and is better positioned to do so. If a user is using the proto definitions, they already have this. If the user isn't using the proto definitions, we're serving them better by providing the stringified form. Adding these maps is a half-solution that serves no one best.

---

pkg/util/metric/metric.go, line 53 at r12 (raw file):

	// GetHelp returns the help text for the metric.
	GetHelp() string
	// GetLabel returns the label for the metric, which describes the entity

Looks like this comment needs updating

---

pkg/util/metric/metric.go, line 56 at r12 (raw file):

	// it measures.
	GetMeasurement() string
	// GetDisplayUnit returns the unit that should be used to display the metric

likewise

---

pkg/util/metric/metric.proto, line 59 at r10 (raw file):

Previously, sploiselle (Sean Loiselle) wrote…

How about "Measurement" and "Unit"? I tired Label, but that felt like it created an ambiguity with GetLabel() and GetLabels().

I think this works well because you could say, "I measured [measurement] in/as a [unit]", e.g. "I measured age in seconds." or "I measured MVCC Values as a count."

Great. The grammarian in me wants it to be "measure" rather than "measurement" but I guess I won't push too hard on that.

---

pkg/util/metric/metric.proto, line 36 at r12 (raw file):

// DisplayUnit describes how the metric's units should be displayed in charts.
enum Unit {
  // Unset expresses that the metric's DisplayUnit wasn't explicitly set.

update comment plz

---

pkg/util/metric/metric.proto, line 48 at r12 (raw file):

  // Percent expresses that the metric's measurement is a percentage value.
  PERCENT = 5;
  // SECONDS expresses that the metric's measurement is in seconds.

Looks like this is the only one in ALL_CAPS

---

pkg/util/metric/metric.proto, line 53 at r12 (raw file):

  // Unix epoch in nanoseconds.
  TIMESTAMP_NS = 7;
  // Timestamp  expresses that the metric's measurement is a time since the 

Looks like both these enum values' comments need updating

---

Comments from Reviewable

[sploiselle]

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/rpc/clock_offset.go, line 55 at r12 (raw file):

Previously, couchand (Andrew Couch) wrote…

not your change but there's a typo here

Done.

---

pkg/server/serverpb/admin.proto, line 561 at r7 (raw file):

Previously, couchand (Andrew Couch) wrote…

We shouldn't be creating this explicitly, since the protobuf toolchain can already provide this functionality and is better positioned to do so. If a user is using the proto definitions, they already have this. If the user isn't using the proto definitions, we're serving them better by providing the stringified form. Adding these maps is a half-solution that serves no one best.

I removed these entirely. I can work on solving this problem when I'm ready to pull the content into the docs site.

---

pkg/util/metric/metric.go, line 53 at r12 (raw file):

Previously, couchand (Andrew Couch) wrote…

Looks like this comment needs updating

Done.

---

pkg/util/metric/metric.go, line 56 at r12 (raw file):

Previously, couchand (Andrew Couch) wrote…

likewise

Done.

---

pkg/util/metric/metric.proto, line 59 at r10 (raw file):

Previously, couchand (Andrew Couch) wrote…

Great. The grammarian in me wants it to be "measure" rather than "measurement" but I guess I won't push too hard on that.

I heavily considered measure but because it's a homonym/graph of the verb, I decided to go with the totally unambiguous noun.

---

pkg/util/metric/metric.proto, line 36 at r12 (raw file):

Previously, couchand (Andrew Couch) wrote…

update comment plz

Done.

---

pkg/util/metric/metric.proto, line 48 at r12 (raw file):

Previously, couchand (Andrew Couch) wrote…

Looks like this is the only one in ALL_CAPS

Done.

---

pkg/util/metric/metric.proto, line 53 at r12 (raw file):

Previously, couchand (Andrew Couch) wrote…

Looks like both these enum values' comments need updating

Done.

---

Comments from Reviewable

[sploiselle]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)