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

Trait for a "label" #297

Open
Palmik opened this issue Apr 22, 2022 · 3 comments
Open

Trait for a "label" #297

Palmik opened this issue Apr 22, 2022 · 3 comments
Labels
C-core Component: core functionality such as traits, etc. C-macros Component: macros. E-complex Effort: complex. T-ergonomics Type: ergonomics. T-experiment Type: experiment.

Comments

@Palmik
Copy link

Palmik commented Apr 22, 2022

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.

@tobz
Copy link
Member

tobz commented Apr 22, 2022

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 tobz added C-macros Component: macros. C-core Component: core functionality such as traits, etc. E-complex Effort: complex. T-experiment Type: experiment. T-ergonomics Type: ergonomics. labels Apr 22, 2022
@Palmik
Copy link
Author

Palmik commented Apr 23, 2022

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.

@tobz
Copy link
Member

tobz commented Apr 24, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-core Component: core functionality such as traits, etc. C-macros Component: macros. E-complex Effort: complex. T-ergonomics Type: ergonomics. T-experiment Type: experiment.
Projects
None yet
Development

No branches or pull requests

2 participants