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

Helper function to build metric's name #25

Open
cristaloleg opened this issue Jun 10, 2021 · 13 comments
Open

Helper function to build metric's name #25

cristaloleg opened this issue Jun 10, 2021 · 13 comments
Labels
enhancement New feature or request

Comments

@cristaloleg
Copy link

Hi, thank you for the lib it's great.

I have a proposition to add a helper function that creates a metric's name by it's name and labels. Current API for Counter(applies to other types too) expects a final name, like foo{bar="baz",aaa="b"}.

Building such names is not that hard but it's repetitive work without sense (had to copy buildName helper 4 or so times between projects).

Proposal: add func BuildName(name string, labels ...string) string that creates a valid Prometheus compatible name. Especially there is no error result, we can just panic if the labels variable isn't even (key-value, huh). However, it may return an error, not so hard too.

Happy to make a PR for that. Thanks.

@valyala
Copy link
Contributor

valyala commented Jun 21, 2021

I'm unsure this helper function is better than fmt.Sprintf() from readability and maintainability points of view. IMHO, fmt.Sprintf("metric_name{label1=%q, ... , labelN=%q}", labelValue1, ..., labelValueN) is easier to read, understand and maintain than metrics.BuildName("metric_name", "label1", labelValue1, ..., "labelN", labelValueN) .

Could you provide a use case, where the metrics.BuildName() is better to use than fmst.Sprintf()?

@valyala valyala added the enhancement New feature or request label Jun 21, 2021
@cristaloleg
Copy link
Author

Sprintf pattern is less discoverable and might be error-prone for the new users, however works in the same way.

Have 0 will to argue further, so closing.

@cristaloleg
Copy link
Author

You know what, I've a case where helper/builder func works better: codegen :)

Instead of combining own helper package for 1 consumer or hacking codegen template, it can be as easy as metrics.BuildName(name, labels...).

What do you think?

@cristaloleg cristaloleg reopened this Jul 15, 2021
@vtolstov
Copy link
Contributor

Yes, as many people have own build name func.
I'm prefer to have sorted labels in metrics

@cristaloleg
Copy link
Author

Indeed, scanning through the code, I've not found labels sorting, looks like foo{bar=val,baz=ue} isn't the same as foo{baz=ue,bar=val} 👀

@vtolstov
Copy link
Contributor

@korjavin
Copy link

I like usage like
metricVec.WithLabelValues("aa","bb").Observe().
In case I have dynamic set of labels (let's say handler name)

How I supposed to handle this in VMmetrics?

@vtolstov
Copy link
Contributor

I like usage like
metricVec.WithLabelValues("aa","bb").Observe().
In case I have dynamic set of labels (let's say handler name)

How I supposed to handle this in VMmetrics?

write own func that get or create needed metric with labels =)

@vtolstov
Copy link
Contributor

link pr #27

@valyala
Copy link
Contributor

valyala commented Jul 21, 2021

Indeed, scanning through the code, I've not found labels sorting, looks like foo{bar=val,baz=ue} isn't the same as foo{baz=ue,bar=val}

Yes, this looks like a low-priority issue, which may be addressed in the future for all the fuctions accepting fully qualified metric name with labels. There is no need in adding BuildName function for resolving this issue.

I'd propose leaving the BuildName function outside the github.com/VictoriaMetrics/metrics package, since:

  • it has higher cognitive complexity compared to fmt.Sprintf due to an extra abstraction level
  • it has no clear advantages over fmt.Sprintf (except of a dedicated function name - then it is better making an alias of fmt.Sprintf with BuildName name :) ).
  • its functionality is specific to a particular use case. For example, in one case it should sort the provided labels, while in other case the labels' sorting isn't needed.

@vtolstov
Copy link
Contributor

Ok

@lammel
Copy link
Contributor

lammel commented Aug 12, 2021

Yes, this looks like a low-priority issue, which may be addressed in the future for all the fuctions accepting fully qualified metric name with labels. There is no need in adding BuildName function for resolving this issue.

I'd propose leaving the BuildName function outside the github.com/VictoriaMetrics/metrics package, since:

  • it has higher cognitive complexity compared to fmt.Sprintf due to an extra abstraction level
  • it has no clear advantages over fmt.Sprintf (except of a dedicated function name - then it is better making an alias of fmt.Sprintf with BuildName name :) ).
  • its functionality is specific to a particular use case. For example, in one case it should sort the provided labels, while in other case the labels' sorting isn't needed.

Having to write the helper to build the label values is currently one of the reasons why we did not start to migrate to metrics in all projects. Probably 90% of our metrics are using label values, which would all need to build the metrics name, so a helper is needed in every project. Not a big hassle, but enough to hold us back it seems.

But if there is need to use a builder for nearly every project, it could be a useful addition to the metrics package to reduce overhead. So from a usability point of view for the developer I think it would be a welcome addition and could help adoption.

Definitely a low prio issue of course.

@wazazaby
Copy link

wazazaby commented Jun 15, 2024

In case anyone is still looking for a performant, type-safe builder for VictoriaMetrics, I made this : https://github.com/wazazaby/vimebu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants