metrics icon indicating copy to clipboard operation
metrics copied to clipboard

Trait for a "label"

Open Palmik opened this issue 3 years ago • 3 comments

I have some type (e.g. struct), that I would like to derive label(s) from, e.g.

struct X {
   abc: u64,
   def: &'static str
}
let x = X { abc: 123, def: "a" };
increment_counter!("foo", x);

Could be equivalent to:

increment_counter!("foo", "x_abc" => 123, "x_def" => "a");

I tried using IntoLabels, unfortunately it does not compose:

increment_counter!("foo", x, y); // Does not work
increment_counter!("foo", x, "x" => "y"); // Does not work

What are your thoughts on extending the macro syntax?

Perhaps another alternative would be to define a macro that would expand values of X into key => value pairs, but I was not able to come up with that -- this would already help and be more efficient (but less ergonomic) as it would also avoid the IntoLabels Vec allocation.

Palmik avatar Apr 22 '22 18:04 Palmik

This is interesting, and could probably be done with a procedural macro that generates code that allows us to transparently generate a &[Label] derived from the struct fields... but you're still going to end up allocating regardless because we need to own the labels, so you're either allocating owned versions of the strings or you're allocating the label container, or both.

I worry a little bit about making it easier to do because we'd be transparently cloning things for people, or doing string formatting, or some other possibly expensive operation, when there's the appearance that they're getting an optimized, tailor-made conversion.

Overall, the design isn't terribly well-adapted to handle non-owned/non-static data for labels, but I'm open to suggestions around that. There's likely a design that could somehow blend the performance benefit of having copy-on-write containers with 'static lifetimes with the ability to use other smart pointer types, such as Arc<T>. That could at least allow for amortizing allocation costs instead of constantly allocating per metric call, etc.

tobz avatar Apr 22 '22 20:04 tobz

Perhaps the trait could be something like:

trait AddLabels {
  // self by value, can be implemented for both X (to allow moving of values) but also for &X
  fn add_labels(self, &mut LabelSet);
}

Where LabelSet is some opaque handle for the existing internal structure for holding labels. This way one could avoid the extra intermediate Vec from IntoLabels and it should be equivalent to passing key => value directly.

Palmik avatar Apr 23 '22 04:04 Palmik

Perhaps the trait could be something like:

trait AddLabels {
  // self by value, can be implemented for both X (to allow moving of values) but also for &X
  fn add_labels(self, &mut LabelSet);
}

Where LabelSet is some opaque handle for the existing internal structure for holding labels. This way one could avoid the extra intermediate Vec from IntoLabels and it should be equivalent to passing key => value directly.

Perhaps. The only way to avoid an allocation is to actually switch to using something like smallvec, or some other allocated-on-the-stack container, that can be promoted to the heap.

With that in mind, we could just as easily create the aforementioned LabelSet as a newtype around Vec<Label>, and add Extend implementations so that callers only had to satisfy Iterator on their side, avoiding a custom trait entirely. This would allow for a future pathway to using smallvec, or something like it, as well.

tobz avatar Apr 24 '22 21:04 tobz