prometheus-cpp icon indicating copy to clipboard operation
prometheus-cpp copied to clipboard

Family::Add better to accept Labels argument by value

Open dmitrmax opened this issue 3 years ago • 0 comments

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)

dmitrmax avatar Jan 15 '23 18:01 dmitrmax