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

Family::Add better to accept Labels argument by value #629

Open
dmitrmax opened this issue Jan 15, 2023 · 0 comments
Open

Family::Add better to accept Labels argument by value #629

dmitrmax opened this issue Jan 15, 2023 · 0 comments

Comments

@dmitrmax
Copy link

dmitrmax commented Jan 15, 2023

Hi!

I've looked how prometheus_cpp is used by me and by other projects and noticed a common pattern when using Family::Add(...):

prometheus::Family<prometheus::Counter>& counter = .... /* Init with builder */
.....
counter.Add({{ "event", "some_event_name }, { "host", "some_host_name"}}).Increment();

I'm not talking about effectiveness of Add from the point of view of creating a unique_ptr with metric before checking that it is already exists - this is covered in one of opened PRs.

I want to talk about prometheus::Labels passed as the first argument. prometheus::Labels is an alias to std::map which is constructed by the caller and later this map serves as a key in the Family::metrics_ map. If Add(...) finds out that this is a new set of labels than it will insert a new element into Family::metrics_ creating a deep copy of supplied Labels with all subsequent allocations of map nodes and strings (if they exceed SSO limit).

I think it would be better to pass Labels by value, so that Family::Add method would like:
template <typename... Args>
T& Add(Labels labels, Args&&... args) {
return Add(std::move(labels), detail::make_unique<T>(args...));
}
Private overload of Family::Add could use emplace and also move these Labels into the key of a new element in Family::metrics_.

I could make a PR, the fix is obvious but I noted reading the discussions that the maintainer wants to preserve the ABI of library (which is good).

So I need to understand if such a PR would be welcomed - what strategy to implement it is OK? Maybe it is better to have a second overload of public Family::Add with the folowing declaration:
template <typename... Args>
T& Add(Labels&& labels, Args&&... args)

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

No branches or pull requests

1 participant