Add constrained labels and Constrained variant for all MetricVecs

[Okhoshi]

Constrained variants of MetricVec provides a way to restrict the possible values a label can take, to prevent cardinality explosion when the label value comes from a non-trusted source (as a user input or HTTP header).

Extracted from discussions on #1066

@bwplotka @kakkoyun

[bwplotka]

Generally great! Just some suggestions ideas for simpler API. Thanks!

[bwplotka]

Some alternatives we might have:

func TestConstrained(t *testing.T) {
	constrained := ConstrainedLabels{
		"c": func(s string) string {
			switch s {
			case "value1":
				return s
			default:
				return "unspecified"
			}
		},
	}

	// Option 1: Wrap registry.
	// Cons: We can't validate WithLabelValues(...)
	{
		r := NewRegistry()
		r = WrapRegistererWithConstrained(constrained, r)
		c := NewCounterVec(CounterOpts{Name: "yolo"}, []string{"a"})
		c2 := NewCounter(CounterOpts{Name: "yolo2"})
		r.MustRegister(c, c2)
	}

	// Option 2a: New function
	// Cons: Weird when we want to have metric with only constrained dynamic labels
	{
		r := NewRegistry()
		c := NewConstrainedCounterVec(CounterOpts{Name: "yolo"}, []string{"a"}, constrained)
		c2 := NewConstrainedCounterVec(CounterOpts{Name: "yolo2"}, nil, constrained)
		r.MustRegister(c, c2)
	}
	// Option 2b: New function with mapping (initial idea)
	// Cons: Complex init code, extra TWO functions to each metric type, not easy to configure mapping between dynamic label name and its constraints, weird UX - might be not clear we are mapping values not adding new ones with the set.
	{
		r := NewRegistry()
		c := NewConstrainedCounterVec(CounterOpts{Name: "yolo"}, []string{"a", "c"}, constrained)
		c2 := NewConstrainedCounterVec(CounterOpts{Name: "yolo2"}, []string{"a"}, constrained)
		r.MustRegister(c, c2)
	}

	// Option 3a: WrapCounter (func WrapCounterWithConstrainedLabel(c interface{}, labels ConstrainedLabels) *CounterVec {)
	// Pros: One wrapper per metric type.
	// Cons: Yet another way to define new label
	{
		r := NewRegistry()
		c := WrapCounterWithConstrainedLabels(NewCounterVec(CounterOpts{Name: "yolo"}, []string{"a"}), constrained)
		c2 := WrapCounterWithConstrainedLabel(NewCounter(CounterOpts{Name: "yolo2"}), constrained)
		r.MustRegister(c, c2)
	}
	// Option 3b: WrapCounter with mapping
	// Pros: One wrapper per metric type.
	// Cons: Yet another way to define new label, same as 2b
	{
		r := NewRegistry()
		c := WrapCounterWithConstrainedLabels(NewCounterVec(CounterOpts{Name: "yolo"}, []string{"a", "b"}), constrained)
		c2 := WrapCounterWithConstrainedLabel(NewCounter(CounterOpts{Name: "yolo2"}), []string{"a"}, constrained)
		r.MustRegister(c, c2)
	}

	// Option 4: New function with redefined API - variadic (perhaps in another package).
	{
		r := NewRegistry()
		c := NewCounterVec2(CounterOpts{Name: "yolo"}, WithCounterVecLabels("a", "b"), WithCounterVecConstrainedLabels(constrained))
		c2 := NewCounterVec2(CounterOpts{Name: "yolo2"}, WithCounterVecLabels("a"), WithCounterVecConstrainedLabels(constrained))
		r.MustRegister(c, c2)
	}

	// Option 5: New function with redefined API - struct (perhaps in another package).
	{
		r := NewRegistry()
		c := NewCounterVec3(CounterVecOpts{Name: "yolo", VariableLabels: []string{"a", "b"}, ConstrainedLabels: constrained})
		c2 := NewCounterVec3(CounterVecOpts{Name: "yolo2", VariableLabels: []string{"a"}, ConstrainedLabels: constrained})
		r.MustRegister(c, c2)
	}
}

type ConstrainedLabel func(string) string
type ConstrainedLabels map[string]ConstrainedLabel

type CounterVecOpts struct {
	Namespace string
	Subsystem string
	Name      string

	Help string

	ConstLabels       Labels
	VariableLabels    []string
	ConstrainedLabels ConstrainedLabels
}

None of the ideas are perfect (: However Option 5 seems to be the cleanest 🤔 arguments for:

Pros:

• Follow client golang patterns (configuring things via opts)
• Extensible

We can also do 5b - so where ConstainedLabels could be change to Constrains of existing Labels in VariableLabels, but to me this is more confusing and harder to implement.

WDYT?

[bwplotka]

Apart from those options I wonder if func(string) string is enough - I wonder if we want another logic to panic/error on certain cardinality. I guess It's fine, error might be handled by implementation of func(string) string through some side effect.

[bwplotka]

cc @beorn7

[Okhoshi]

Apart from those options I wonder if func(string) string is enough - I wonder if we want another logic to panic/error on certain cardinality. I guess It's fine, error might be handled by implementation of func(string) string through some side effect.

For me, the logic to not return error was that we always reduce to some known values. If some values are completely unacceptable, there's still the option to panic inside the func(string) string or to validate the values before passing them to the WithLabels.

[Okhoshi]

Some alternatives we might have:

[code stripped for readability]

None of the ideas are perfect (: However Option 5 seems to be the cleanest 🤔 arguments for:

Pros:

• Follow client golang patterns (configuring things via opts)
• Extensible

We can also do 5b - so where ConstainedLabels could be change to Constrains of existing Labels in VariableLabels, but to me this is more confusing and harder to implement.

WDYT?

I like Option 4 and 5, for their extensibility and that they are known patterns. The choice between the two is essentially a question of personal taste I guess. We could have them in a separate package until v2 where we could make them default, WDYT ?

I also agree on the better readability if we don't merge variableLabels and constrainedLabels, having them completely separated and panicking if they overlap seems the most predictable and understandable way to manage them.
In that case, we only need to define the behaviour of WithLabelValues since one doesn't the label names, only the values. The most logical way would be to keep the ordering of variableLabels and then the ordering of constrainedLabels.

Just to make sure I'm on the same page as you, constrainedLabels are not applicable to the single Metrics types (Counter, Gauge, ...), are they ?

[bwplotka]

I like Option 4 and 5, for their extensibility and that they are known patterns. The choice between the two is essentially a question of personal taste I guess. We could have them in a separate package until v2 where we could make them default, WDYT ?

Yea, I think Opt 5 is equally explicit and less verbose perhaps, so probably better?

In that case, we only need to define the behaviour of WithLabelValues since one doesn't the label names, only the values. The most logical way would be to keep the ordering of variableLabels and then the ordering of constrainedLabels.

You are totally right, this might cause confusion... I thought as you proposed, but it's not very explicit (someone might add constainedLabel fields above variable label fields in their definition). This is definitely Pros of your approach of setting constraints to existing variable labels. Another trade-off (:

Just to make sure I'm on the same page as you, constrainedLabels are not applicable to the single Metrics types (Counter, Gauge, ...), are they ?

If we assume non-mapping approach (separate label names to variableLabels) - they are not applicable to non Vectors.

[Okhoshi]

Yea, I think Opt 5 is equally explicit and less verbose perhaps, so probably better?

I agree, let's start with Opt5 then 👍

You are totally right, this might cause confusion... I thought as you proposed, but it's not very explicit (someone might add constainedLabel fields above variable label fields in their definition). This is definitely Pros of your approach of setting constraints to existing variable labels. Another trade-off (:

Definitely not an easy task to define this new API 😅
Instead of splitting them completely, we could also merge them completely. And since we opt for Opt5, it gives us some freedom to redefine how we achieve this.

type ConstrainedLabel struct {
  Name         string
  Constraint func(string)string
}

type CounterVecOpts struct {
  CounterOpts
  VariableLabels []ConstrainedLabel
}

// ---

prometheus.V2().NewCounterVec(CounterVecOpts{
  ..., // Usual CounterOpts stuff
  VariableLabels: []ConstrainedLabels{
    {Name: "A"},
    {Name: "B", Constraint: func(v string) string { ... }},
  },
})

Compared to the suggestion I made on Slack, I moved to an array because maps do not preserve key order.

WDYT ?

[beorn7]

I was too busy to look at this all week. Maybe I'll find time next week. But feel free to move forward without me.

[beorn7]

Finally I had a bit of time to look at it.

I haven't checked correctness (i.e. "is it doing what we expect it to do?"). I assume you are on top of that anyway.

Interesting approach with the V2 variable. Do you know of any other applications of this pattern? It looks intriguing, but it would be even better if this has already been tried elsewhere.

In general, I'm OK with this. I'll elaborate on one more idea in the next comment, but feel free to go ahead as is.

[beorn7]

This whole thing reminds me of ancient thoughts I had about revamping labels. One was about somehow automating the instantiation of all the possible vector elements, which is something that is recommended to do manually right now. Here is an example from the client_golang code itself:

	cnt := prometheus.NewCounterVec(
		prometheus.CounterOpts{
			Name: "promhttp_metric_handler_requests_total",
			Help: "Total number of scrapes by HTTP status code.",
		},
		[]string{"code"},
	)
	// Initialize the most likely HTTP status codes.
	cnt.WithLabelValues("200")
	cnt.WithLabelValues("500")
	cnt.WithLabelValues("503")

Now imagine the code label above being a ConstrainedLabel that normalizes all label values to either 200, 500, 503, or other.

Imagine further that the ConstrainedLabel type looks like this:

type ConstrainedLabel struct {
	Name       string
	Constraint func(string) string
	Values     func() []string
}

If Values returns a non-empty slice, the slice contains all possible label values.

And now we have all the parts in place: If a vector has only constant and constrained labels, of which all have a Values function that returns a non-empty slice, the code can automatically create the cartesian product of all labels and initialize all vector elements.

WDYT?

[Okhoshi]

And now we have all the parts in place: If a vector has only constant and constrained labels, of which all have a Values function that returns a non-empty slice, the code can automatically create the cartesian product of all labels and initialize all vector elements.

WDYT?

I really loves the idea ! Instantiating all the possibles combinations always add some boilerplate code we could indeed automate in this case.

However, I'm not really sure to see how we could enforce that Constraint won't ever return something else than an element of Values ?
And is there any reason to make Values a function that returns []string instead of the slice itself ?

Also, there's situation where the Cartesian product of all labels is larger than the actual set of label combinations, because some combinations may be impossible, we couldn't do anything about that though.

[bwplotka]

Hm, I like the idea of reusing some concepts around instantiation, although arguably AFAIK those concepts might go away if we implement _created timestamp in OM. 🙈

On top of that, automation feels like the second step. The first should be to define the API for constraints. I am a bit afraid of putting Values items. Do we really need to force everyone to expose hundreds status code metrics, because constraints allow max 100, when during execution only 200 and 500 codes were seen for weeks, so only 2 metrics could suffice?

[beorn7]

However, I'm not really sure to see how we could enforce that Constraint won't ever return something else than an element of Values ?

That would be the contract of the interface. Go's type system is not powerful enough to let the compiler enforce it, but that shouldn't keep us from using such a contract, should it?

And is there any reason to make Values a function that returns []string instead of the slice itself ?

I guess the reason is that I thought of ConstrainedLabel as an interface, but it is, in fact, a struct. (Maybe it should be an interface? Not sure…) So yes, if ConstrainedLabel remains a struct, with the name as a field (rather than a method), then Values should just be a field, too.

On top of that, automation feels like the second step. The first should be to define the API for constraints.

Sure. But we should design the API now in a way that it allows this kind of automation without a breaking change.
(Which means we might need a boolean to switch the behavior on explicitly if we introduce it later.)

Do we really need to force everyone to expose hundreds status code metrics, because constraints allow max 100, when during execution only 200 and 500 codes were seen for weeks, so only 2 metrics could suffice?

First of all, we could make the auto-initialization an opt-in, see above.

But to think a bit further: In the cleanest Prometheus semantics, you want to initialize all metrics in a vector in advance. Having "rare" status codes only show up when they actually occur is indeed a smell (the "springing into existence with a value of one" problem is not the only problem with that). In practice, we cannot always predict all future label values in advance, so we have to live with the situation. However, with constrained labels in the game, we do know all possible label values in advance, so we can initialize the full metrics vector. I would also argue that the situation where you require the label values to be constrained is likely also a situation where you want clean semantics.

On the other hand, if you want to use a constrained label merely to check if an HTTP status code is "syntactically correct" (three digit number), then you wouldn't populate the Values slice anyway, and the auto-init wouldn't kick in.

If you want to use a constrained label to make sure it only has a very constrained set of label values (e.g. 200, 500, other), then you would populate the Values slice and get the auto-init out of the box.

So maybe we don't need an opt-in flag, if we say you "opt in" by populating the Values slice.

[Okhoshi]

I guess the reason is that I thought of ConstrainedLabel as an interface, but it is, in fact, a struct. (Maybe it should be an interface? Not sure…) So yes, if ConstrainedLabel remains a struct, with the name as a field (rather than a method), then Values should just be a field, too.

I don't really see any reason to make ConstrainedLabel an interface, but would be happy to be proven wrong 👍

But we should design the API now in a way that it allows this kind of automation without a breaking change.

It seems to me that the current design is flexible enough to be extended in the future to support such automation.

Could we go further with this MR as is then ?

[beorn7]

Could we go further with this MR as is then ?

Seems fine to me.

[bwplotka]

Looks clean! Some comments otherwise LGTM. I asked on Gopher slack about out V2 pattern - maybe we can get some quick feedback.

Thanks 💪🏽

cc @kakkoyun

[bwplotka]

Amazing work, thanks!

Important comment about V2 struct is missing, otherwise it's good to merge, thanks!

[bwplotka]

💪🏽