Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add BuildName helper func #27

Closed
wants to merge 4 commits into from
Closed

add BuildName helper func #27

wants to merge 4 commits into from

Conversation

vtolstov
Copy link
Contributor

Signed-off-by: Vasiliy Tolstov v.tolstov@unistack.org

Signed-off-by: Vasiliy Tolstov <v.tolstov@unistack.org>
metrics_test.go Outdated Show resolved Hide resolved
metrics.go Show resolved Hide resolved
metrics.go Outdated Show resolved Hide resolved
metrics.go Show resolved Hide resolved
metrics.go Show resolved Hide resolved
Signed-off-by: Vasiliy Tolstov <v.tolstov@unistack.org>
Signed-off-by: Vasiliy Tolstov <v.tolstov@unistack.org>
@vtolstov
Copy link
Contributor Author

@valyala @tenmozes can you merge this if it's ok now?

@vtolstov
Copy link
Contributor Author

gentle ping..

Copy link
Contributor

@valyala valyala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see #25 (comment)

metrics.go Outdated Show resolved Hide resolved
Signed-off-by: Vasiliy Tolstov <v.tolstov@unistack.org>
@hagen1778
Copy link
Contributor

hagen1778 commented Aug 5, 2021

Hi @vtolstov! What would be the typical use case for the new helper?
If it is:

metrics.NewSummary(metrics.BuildName("my_metric", "zerolabel", "value3", ",firstlabel", "value2"))

then metric name validation happens twice.

If it is:

myMetricName := metrics.BuildName("my_metric", "zerolabel", "value3", ",firstlabel", "value2")
myMetric := metrics.NewSummary(myMetricName)

then it is unclear why metrics.BuildName panics instead of returning an error for processing.

IMHO, in the role of helper I'd prefer smthng like the following:

metrics.NewSummaryWithLabels("my_metric", "zerolabel", "value3", ",firstlabel", "value2")

or

metrics.NewSummary("my_metric").WithLabels("zerolabel", "value3", ",firstlabel", "value2")

wdyt?

// disclaimer: i do not maintain this repo, just out of my curiosity

@vtolstov
Copy link
Contributor Author

vtolstov commented Aug 5, 2021

@hagen1778 may be you case is better, but for now i'm move helper to own framework =)

@vtolstov vtolstov closed this Sep 11, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants