Skip to content

Commit

Permalink
fix(core): gracefully handle label hash collisions
Browse files Browse the repository at this point in the history 
```
before:

BM_Registry_CreateFamily             49.2 ns         49.0 ns     11087352
BM_Registry_CreateCounter/0           165 ns          165 ns      4170191
BM_Registry_CreateCounter/1          1967 ns         1958 ns       556346
BM_Registry_CreateCounter/8          4280 ns         4269 ns       176711
BM_Registry_CreateCounter/64        27091 ns        27009 ns        25755
BM_Registry_CreateCounter/512      224201 ns       223581 ns         3023
BM_Registry_CreateCounter/4096    2056280 ns      2049876 ns          330

after:

BM_Registry_CreateFamily             47.8 ns         47.8 ns     15131733
BM_Registry_CreateCounter/0           158 ns          158 ns      4295006
BM_Registry_CreateCounter/1          1009 ns         1009 ns      1169219
BM_Registry_CreateCounter/8          3238 ns         3231 ns       203179
BM_Registry_CreateCounter/64        25989 ns        25949 ns        27008
BM_Registry_CreateCounter/512      211288 ns       211220 ns         3265
BM_Registry_CreateCounter/4096    1960240 ns      1959743 ns          350
```

Fixes: #532
  • Loading branch information
@gjasny
gjasny committed Nov 13, 2021
1 parent 7aaecba commit 7075a2b
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 58 deletions.
19 changes: 10 additions & 9 deletions core/include/prometheus/detail/utils.h
View file
Expand Up @@ -7,17 +7,18 @@
#include "prometheus/detail/core_export.h"

namespace prometheus {

namespace detail {

/// \brief Compute the hash value of a map of labels.
///
/// \param labels The map that will be computed the hash value.
///
/// \returns The hash value of the given labels.
PROMETHEUS_CPP_CORE_EXPORT std::size_t hash_labels(
const std::map<std::string, std::string>& labels);
/// \brief Label hasher for use in STL containers.
struct PROMETHEUS_CPP_CORE_EXPORT LabelHasher {
/// \brief Compute the hash value of a map of labels.
///
/// \param labels The map that will be computed the hash value.
///
/// \returns The hash value of the given labels.
std::size_t operator()(
const std::map<std::string, std::string>& labels) const;
};

} // namespace detail

} // namespace prometheus
11 changes: 6 additions & 5 deletions core/include/prometheus/family.h
View file
@@ -1,6 +1,5 @@
#pragma once

#include <cstddef>
#include <map>
#include <memory>
#include <mutex>
Expand All @@ -12,6 +11,7 @@
#include "prometheus/collectable.h"
#include "prometheus/detail/core_export.h"
#include "prometheus/detail/future_std.h"
#include "prometheus/detail/utils.h"
#include "prometheus/metric_family.h"

// IWYU pragma: no_include "prometheus/counter.h"
Expand Down Expand Up @@ -142,16 +142,17 @@ class PROMETHEUS_CPP_CORE_EXPORT Family : public Collectable {
std::vector<MetricFamily> Collect() const override;

private:
std::unordered_map<std::size_t, std::unique_ptr<T>> metrics_;
std::unordered_map<std::size_t, std::map<std::string, std::string>> labels_;
std::unordered_map<T*, std::size_t> labels_reverse_lookup_;
std::unordered_map<std::map<std::string, std::string>, std::unique_ptr<T>,
detail::LabelHasher>
metrics_;

const std::string name_;
const std::string help_;
const std::map<std::string, std::string> constant_labels_;
mutable std::mutex mutex_;

ClientMetric CollectMetric(std::size_t hash, T* metric) const;
ClientMetric CollectMetric(const std::map<std::string, std::string>& labels,
T* metric) const;
T& Add(const std::map<std::string, std::string>& labels,
std::unique_ptr<T> object);
};
Expand Down
3 changes: 2 additions & 1 deletion core/src/detail/utils.cc
View file
Expand Up @@ -8,7 +8,8 @@ namespace prometheus {

namespace detail {

std::size_t hash_labels(const std::map<std::string, std::string>& labels) {
std::size_t LabelHasher::operator()(
const std::map<std::string, std::string>& labels) const {
size_t seed = 0;
for (auto& label : labels) {
hash_combine(&seed, label.first, label.second);
Expand Down
64 changes: 28 additions & 36 deletions core/src/family.cc
View file
Expand Up @@ -8,7 +8,6 @@

#include "prometheus/check_names.h"
#include "prometheus/counter.h"
#include "prometheus/detail/utils.h"
#include "prometheus/gauge.h"
#include "prometheus/histogram.h"
#include "prometheus/summary.h"
Expand All @@ -33,55 +32,47 @@ Family<T>::Family(const std::string& name, const std::string& help,
template <typename T>
T& Family<T>::Add(const std::map<std::string, std::string>& labels,
std::unique_ptr<T> object) {
const auto hash = detail::hash_labels(labels);
std::lock_guard<std::mutex> lock{mutex_};
auto metrics_iter = metrics_.find(hash);

if (metrics_iter != metrics_.end()) {
#ifndef NDEBUG
auto labels_iter = labels_.find(hash);
assert(labels_iter != labels_.end());
const auto& old_labels = labels_iter->second;
assert(labels == old_labels);
#endif
return *metrics_iter->second;
}

for (auto& label_pair : labels) {
const auto& label_name = label_pair.first;
if (!CheckLabelName(label_name)) {
throw std::invalid_argument("Invalid label name");
}
if (constant_labels_.count(label_name)) {
throw std::invalid_argument("Duplicate label name");
auto insert_result =
metrics_.insert(std::make_pair(labels, std::move(object)));

if (insert_result.second) {
// insertion took place, retroactively check for unlikely issues
for (auto& label_pair : labels) {
const auto& label_name = label_pair.first;
if (!CheckLabelName(label_name)) {
metrics_.erase(insert_result.first);
throw std::invalid_argument("Invalid label name");
}
if (constant_labels_.count(label_name)) {
metrics_.erase(insert_result.first);
throw std::invalid_argument("Duplicate label name");
}
}
}

const auto metric = metrics_.insert(std::make_pair(hash, std::move(object)));
assert(metric.second);
labels_.insert({hash, labels});
labels_reverse_lookup_.insert({metric.first->second.get(), hash});
return *(metric.first->second);
auto& stored_object = insert_result.first->second;
assert(stored_object);
return *stored_object;
}

template <typename T>
void Family<T>::Remove(T* metric) {
std::lock_guard<std::mutex> lock{mutex_};
if (labels_reverse_lookup_.count(metric) == 0) {
return;
}

const auto hash = labels_reverse_lookup_.at(metric);
metrics_.erase(hash);
labels_.erase(hash);
labels_reverse_lookup_.erase(metric);
for (auto it = metrics_.begin(); it != metrics_.end(); ++it) {
if (it->second.get() == metric) {
metrics_.erase(it);
break;
}
}
}

template <typename T>
bool Family<T>::Has(const std::map<std::string, std::string>& labels) const {
const auto hash = detail::hash_labels(labels);
std::lock_guard<std::mutex> lock{mutex_};
return metrics_.find(hash) != metrics_.end();
return metrics_.count(labels) != 0u;
}

template <typename T>
Expand Down Expand Up @@ -114,8 +105,10 @@ std::vector<MetricFamily> Family<T>::Collect() const {
}

template <typename T>
ClientMetric Family<T>::CollectMetric(std::size_t hash, T* metric) const {
ClientMetric Family<T>::CollectMetric(
const std::map<std::string, std::string>& metric_labels, T* metric) const {
auto collected = metric->Collect();
collected.label.reserve(constant_labels_.size() + metric_labels.size());
const auto add_label =
[&collected](const std::pair<std::string, std::string>& label_pair) {
auto label = ClientMetric::Label{};
Expand All @@ -124,7 +117,6 @@ ClientMetric Family<T>::CollectMetric(std::size_t hash, T* metric) const {
collected.label.push_back(std::move(label));
};
std::for_each(constant_labels_.cbegin(), constant_labels_.cend(), add_label);
const auto& metric_labels = labels_.at(hash);
std::for_each(metric_labels.cbegin(), metric_labels.cend(), add_label);
return collected;
}
Expand Down
20 changes: 13 additions & 7 deletions core/tests/utils_test.cc
View file
Expand Up @@ -9,26 +9,32 @@ namespace prometheus {

namespace {

TEST(UtilsTest, hash_labels_1) {
class UtilsTest : public testing::Test {
public:
detail::LabelHasher hasher;
};

TEST_F(UtilsTest, hash_labels_1) {
std::map<std::string, std::string> labels;
labels.insert(std::make_pair<std::string, std::string>("key1", "value1"));
labels.insert(std::make_pair<std::string, std::string>("key2", "vaule2"));
auto value1 = detail::hash_labels(labels);
auto value2 = detail::hash_labels(labels);

auto value1 = hasher(labels);
auto value2 = hasher(labels);

EXPECT_EQ(value1, value2);
}

TEST(UtilsTest, hash_labels_2) {
TEST_F(UtilsTest, hash_labels_2) {
std::map<std::string, std::string> labels1{{"aa", "bb"}};
std::map<std::string, std::string> labels2{{"a", "abb"}};
EXPECT_NE(detail::hash_labels(labels1), detail::hash_labels(labels2));
EXPECT_NE(hasher(labels1), hasher(labels2));
}

TEST(UtilsTest, hash_label_3) {
TEST_F(UtilsTest, hash_label_3) {
std::map<std::string, std::string> labels1{{"a", "a"}};
std::map<std::string, std::string> labels2{{"aa", ""}};
EXPECT_NE(detail::hash_labels(labels1), detail::hash_labels(labels2));
EXPECT_NE(hasher(labels1), hasher(labels2));
}

} // namespace
Expand Down

0 comments on commit 7075a2b

Please sign in to comment.