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

add variants of the VictoriaMetrics builders #25

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

vrischmann
Copy link
Contributor

In some cases we need to create a metric in a specific metrics.Set, for example if you want to unregister a bunch of metrics at some point the easiest way to do that is to use a dedicated set and use metrics.UnregisterSet.

This PR adds variants of the VM helpers that allow creating the metric in a specific set instead of the global set.

Take an explicit *metrics.Set instead of using the default
@wazazaby wazazaby self-requested a review January 9, 2024 08:59
Copy link
Owner

@wazazaby wazazaby left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for your contribution.

Could you please update all the godoc to follow what I described in my comment ?

Also, looking at the metrics.Set type and it's methods, there's no nil-check meaning we will run into a NPE if a nil set is passed. I wonder if we should add a nil-check in these methods, what do you think ?

victoria_metrics.go Outdated Show resolved Hide resolved
@vrischmann
Copy link
Contributor Author

Also, looking at the metrics.Set type and it's methods, there's no nil-check meaning we will run into a NPE if a nil set is passed. I wonder if we should add a nil-check in these methods, what do you think ?

In my opinion this is a case where providing a nil set is unexpected, aka a programmer error.

If we get a nil set we only have two options:

  • return a nil metric type
  • panic

In the first case, it would mean that the caller will either:

  • have to handle this with a nil-check, but at this point why not just nil-check the set before calling ?
  • panic trying to call a method on the metric, which is nil

In the second case, there's not much point using a custom panic since it essentially has the same meaning as a runtime nil-panic.

@wazazaby
Copy link
Owner

wazazaby commented Jan 9, 2024

Agreed, thanks for the analysis.

Let's keep is this way.

@wazazaby wazazaby self-requested a review January 9, 2024 10:57
Copy link
Owner

@wazazaby wazazaby left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@wazazaby wazazaby merged commit efd26c5 into wazazaby:master Jan 9, 2024
1 check passed
@vrischmann vrischmann deleted the new-set branch January 9, 2024 10:59
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.

2 participants