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

Promsafe: Strongly-typed safe labels #1598

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

amberpixels
Copy link

@amberpixels amberpixels commented Aug 28, 2024

Promsafe

Introducing promsafe lib (optional helper lib, similar to promauto) that allows to use type-safe labels.

Motivation

This PR only covers Counter functionality as an example. If idea is fine for community, I'll push further commits expanding logic to Gauge, Histogram, etc
For detailed motivation see my comment below

Fixes #1599

Why?

Currently having unsafe labels lead to several problems: either err-handling nightmare, either panicing (in case if you use "promauto")

Having unsafe labels can lead to following issues:

  • Misspelling
  • Mistyping
  • Misremembering
  • Too many labels
  • Too few labels

As of state of art of modern Go version, we can use Go Generics to solve these issues.

Examples of how to use it

1. Single-label mode (no need to introduce a struct here, we can use a typed string directly)

	c := promsafe.NewCounterVecT(prometheus.CounterOpts{
		Name: "items_counted_by_status",
	}, promsafe.SingleLabelProvider("status"))

	// Manually register the counter
	if err := prometheus.Register(c.Unsafe()); err != nil {
		log.Fatal("could not register: ", err.Error())
	}

        // It only allows you to give ONE string here
	c.With("active").Inc()

2. Multi-label mode (safe structs)

	type MyCounterLabels struct {
		promsafe.StructLabelProvider
		EventType string
		Success   bool
		Position  uint8 // yes, it's a number, but be careful with high-cardinality labels

		ShouldNotBeUsed string `promsafe:"-"`
	}

	c := promsafe.NewCounterVecT(prometheus.CounterOpts{
		Name: "items_counted",
	}, &MyCounterLabels{})

	// Manually register the counter
	if err := prometheus.Register(c.Unsafe()); err != nil {
		log.Fatal("could not register: ", err.Error())
	}

	//  it ONLY allows you to fill the &MyCounterLabels here
        c.With(&MyCounterLabels{
		EventType: "request", Success: true, Position: 1,
	}).Inc()

Compatibility with promauto

1. promauto.With call migration

	var myReg = prometheus.NewRegistry()

	counterOpts := prometheus.CounterOpts{
		Name: "items_counted",
	}

	// Old unsafe code
	// promauto.With(myReg).NewCounterVec(counterOpts, []string{"event_type", "source"})
	// becomes:

	type TicketReservationAttemptsLabels struct {
		promsafe.StructLabelProvider
		EventType string
		Source    string
	}
	c := promsafe.WithAuto(myReg).NewCounterVecT(counterOpts, &TicketReservationAttemptsLabels{})

	c.With(&TicketReservationAttemptsLabels{
		EventType: "reservation", Source: "source1",
	}).Inc()

2. Global promauto setup (all New* calls will behave same as promauto.New*)

	// Setup so every NewCounter* call will use default registry
	// like promauto does
	// Note: it actually accepts other registry to become a default one
	promsafe.SetupGlobalPromauto()

	counterOpts := prometheus.CounterOpts{
		Name: "items_counted",
	}

	// Old code:
	//c := promauto.NewCounterVec(counterOpts, []string{"status", "source"})
	//c.With(prometheus.Labels{
	//	"status": "active",
	//	"source": "source1",
	//}).Inc()
	// becomes:

	type TicketReservationAttemptsLabels struct {
		promsafe.StructLabelProvider
		Status string
		Source string
	}
	c := promsafe.NewCounterVecT(counterOpts, &TicketReservationAttemptsLabels{})

	c.With(&TicketReservationAttemptsLabels{
		Status: "active", Source: "source1",
	}).Inc()

@amberpixels amberpixels changed the title Promsafe feature introduced Promsafe: Strongly-typed safe labels Aug 28, 2024
@amberpixels amberpixels force-pushed the feature/promsafe branch 3 times, most recently from c064fc4 to 83aba46 Compare August 28, 2024 16:11
Signed-off-by: Eugene <eugene@amberpixels.io>
Signed-off-by: Eugene <eugene@amberpixels.io>
@bwplotka
Copy link
Member

bwplotka commented Sep 3, 2024

Hi! Thanks for innovating here 💪🏽

I presume this is about using generics for label values type safety -- in the relation to defined label names.

Currently having unsafe labels lead to several problems: either err-handling nightmare, either panicing (in case if you use "promauto")

Can you share exactly the requirements behind promsafe. Perhaps it would allow us to make decision if such package is useful to enough to maintain in client_golang OR existing solutions are enough OR is there a way to extend existing packages with improvements for the same goals.

For example, how often you see those err handling nightmare and panics in practice? Can you share some experience/data?

Generally, what's recommended is hardcoding label values in WithLabelValues, which by testing given code-path you know immediately if it's panicking. If you use dynamic values (e.g. variable) as your values then it's generally prone to cardinality issues anyway, thus we experimented with constraint labels solution.

Thus, let's circle back to barebone requirements we want here 🤗 e.g. generally you should avoid using With. What are the cases we are solving here?

Additionally, performance is important for this increment flow, so it would be nice to check how this applies.

@amberpixels
Copy link
Author

amberpixels commented Sep 4, 2024

Hey. Thanks for a feedback. Let me share details on my motivation behind the provided promsafe package.

By err handling nightmare / panics I meant the following cases:

// Counter registration: we're fine with possible panic here :)
myCounter := promauto.NewCounterVec(prometheus.CounterOpts{
    Name: "items_counted",
}, []string{"event_type", "success", "slot" /* 1/2/3/.../9 */})

// But using counter: where there motivation comes from:

// Using .GetMetricWith*() methods will error if labels are messed up
myCounterWithValues, err := myCounter.GetMetricWith(prometheus.Labels{
    "event_type": "reservation",
    "success":    "true",
    "slot":       "1",
})
if err != nil {
    // TODO: handle error
}
// Same error can happen if using *WithLabelValues version:
// myCounterWithValues, err := myCounter.GetMetricWithLabelValues("reservation", "true", "1")

// To avoid error-handling we can use .With/.WithLabelValues, but it will just panic for the same reasons:
myCounter.WithLabelValues("reservation", "true", "2").Inc()

💡 So here and further i call "panic" both panicing of .With* methods or error-handling in .GetMetricWith* methods

Why Panic? why it matters?

Here are several reasons:

  1. Misspelling. You can misspell label names. (Not relevant for WithLabelValues though)
  2. Misremembering. You can forget the name of the label. In case of using WithLabelValues you still need to remember the number of labels and their order (and what they mean)
  3. Missing labels. You can forget a label (both in map of With() or in slice of WithLabelValues())
  4. Extra labels. You can accidentally pass extra labels (both in map of With() or in slice of WithLabelValues())
  5. Manual string conversion can lead to failures as well. E.g. you must know to use fmt.Sprintf("%v", boolValue), and choosing wrong "%v" placeholder can ruin values.

All these reasons are possible ways to break code because of panicking in .With() or .WithLabelValues(). Let's not spend time and efforts on code-reviews to ensure that new usage of "counter inc" is not breaking everything.

Also, one more reason is not about failing but about consistency:
6. Type-safety allows you to be both less error-prone and more consistent.
E.g. you just pass bool values as label values, and you know it will always be "true"/"false" not "1","0","on","off","yes",...
same for numbers

How it's solved by promsafe?

// Promsafe example:
// Registering a metric with simply providing the type containing labels
type MyCounterLabels struct {
    promsafe.StructLabelProvider
    EventType string
    Success   bool
    Slot      uint8 // yes, it's a number, still should be careful with high-cardinality labels
}
myCounterSafe := promsafe.NewCounterVecT[MyCounterLabels](prometheus.CounterOpts{
    Name: "items_counted_detailed",
})

// Calling counter is simple: just provide the filled struct of the dedicated type.
//
// Neither of 5 reasons can panic here. You simply can't mess up the struct.
// With() accepts ONLY this type of struct, you can't send any other struct.
// You don't need to remember the fields and their order. IDE will show you them.
// You can't send more fields.
// You can send less fields (but it can easily fill up with default values, or other custom non-panicy logic).
// You can't mess up types.
// You're consistent with types.
myCounterSafe.With(MyCounterLabels{
    EventType: "reservation", Success: true, Slot: 1,
}).Inc()

P.S. issue with inconsistency of promsafe-version of WithLabelValues() method

// One thing that I need to specify here is the inconsistency with promsafe-version of WithLabelValues()

// In an ideal world `myCounterSafe` from example above  SHOULD work like that (with fixed typed arguments)
myCounterSafe.WithLabelValues("reservation", true, 1).Inc()

// But it's not possible to implement this in a completely type-safe way with modern Go. (i don't consider go-generate here)
// That's why that was the reason to simply not use this method, and make it be EXACTLY the same as With()
// Here it is the subject to be discussed.

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.

Type-safe labels support?
2 participants