Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ Increment the:
* [CONFIGURATION] Add support for the composite sampler configuration
(programmatic and from yaml)
([#4366](https://github.com/open-telemetry/opentelemetry-cpp/pull/4366))

* [SDK] Replace SpinLockMutex with std::mutex in the metrics library
[#4416](https://github.com/open-telemetry/opentelemetry-cpp/pull/4416)

* [SDK] `OTELResourceDetector` now percent-decodes values parsed from the
`OTEL_RESOURCE_ATTRIBUTES` environment variable, per the W3C Baggage value
grammar the resource spec defers to. A malformed escape sequence is left
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

#include <cstdint>
#include <memory>
#include <mutex>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h"
#include "opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_indexer.h"
Expand Down Expand Up @@ -49,7 +49,7 @@ class Base2ExponentialHistogramAggregation : public Aggregation
double value) noexcept;
void Downscale(uint32_t by) noexcept;

mutable opentelemetry::common::SpinLockMutex lock_;
mutable std::mutex lock_;
Base2ExponentialHistogramPointData point_data_;
Base2ExponentialHistogramIndexer indexer_;
bool record_min_max_ = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
#include <cstddef>
#include <cstdint>
#include <memory>
#include <mutex>
#include <vector>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/nostd/variant.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h"
Expand Down Expand Up @@ -49,7 +49,7 @@ class LongHistogramAggregation : public Aggregation
PointType ToPoint() const noexcept override;

private:
mutable opentelemetry::common::SpinLockMutex lock_;
mutable std::mutex lock_;
HistogramPointData point_data_;
bool record_min_max_ = true;
};
Expand Down Expand Up @@ -79,7 +79,7 @@ class DoubleHistogramAggregation : public Aggregation
PointType ToPoint() const noexcept override;

private:
mutable opentelemetry::common::SpinLockMutex lock_;
mutable std::mutex lock_;
mutable HistogramPointData point_data_;
bool record_min_max_ = true;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

#include <cstdint>
#include <memory>
#include <mutex>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
#include "opentelemetry/sdk/metrics/data/metric_data.h"
#include "opentelemetry/sdk/metrics/data/point_data.h"
Expand Down Expand Up @@ -34,7 +34,7 @@ class LongLastValueAggregation : public Aggregation
PointType ToPoint() const noexcept override;

private:
mutable opentelemetry::common::SpinLockMutex lock_;
mutable std::mutex lock_;
LastValuePointData point_data_;
};

Expand All @@ -55,7 +55,7 @@ class DoubleLastValueAggregation : public Aggregation
PointType ToPoint() const noexcept override;

private:
mutable opentelemetry::common::SpinLockMutex lock_;
mutable std::mutex lock_;
mutable LastValuePointData point_data_;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

#include <cstdint>
#include <memory>
#include <mutex>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
#include "opentelemetry/sdk/metrics/data/metric_data.h"
#include "opentelemetry/sdk/metrics/data/point_data.h"
Expand Down Expand Up @@ -35,7 +35,7 @@ class LongSumAggregation : public Aggregation
PointType ToPoint() const noexcept override;

private:
mutable opentelemetry::common::SpinLockMutex lock_;
mutable std::mutex lock_;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've made the swap to std::mutex here also to be consistent throughout the SDK. This use case for sum aggregation could be a good use for a spinlock but testing showed it was ~10ns difference with the mutex in the uncontested case. This performance delta is not significant compared to the attribute hash map lookup.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The attribute-map comparison makes sense for the unbound path, but the bound path skips that lookup. It now takes the BoundEntry mutex and then this mutex for every sum update, so the reported 10 ns may matter more there.

Could we capture before-and-after results for the existing bound and unbound counter benchmarks, including a contended case? Since #4317 originally kept this spinlock and requested benchmarks for part 2, I think we should have that evidence before changing this hot path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could increase memory usage significantly. Measured with MSVC x64 but it should similar for Linux.

type before (bytes) after (bytes)
LongSumAggregation 40 112
LongLastValueAggregation 48 120
LongHistogramAggregation 136 208

The driver is that sizeof(std::mutex) is 80 bytes on MSVC and 40 on glibc, while sizeof(std::atomic<bool>) is 1. One aggregation object is allocated per time series, and the default cardinality limit is 2000. That alone is 2000 x 72 = ~144 KB extra per counter instrument. TemporalMetricStorage keeps a per-collector stash of previously reported aggregations, so a configuration with one reader roughly doubles it, reaching near 300 KB per instrument.

SumPointData point_data_;
};

Expand All @@ -56,7 +56,7 @@ class DoubleSumAggregation : public Aggregation
PointType ToPoint() const noexcept override;

private:
mutable opentelemetry::common::SpinLockMutex lock_;
mutable std::mutex lock_;
SumPointData point_data_;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora
// Async counter always record monotonically increasing values, and the
// exporter/reader can request either for delta or cumulative value.
// So we convert the async counter value to delta before passing it to temporal storage.
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(hashmap_lock_);
std::lock_guard<std::mutex> guard(hashmap_lock_);
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
const bool offer_exemplars =
ExemplarFilterEnabled(exemplar_filter_type_, opentelemetry::context::Context{});
Expand Down Expand Up @@ -130,7 +130,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora

std::shared_ptr<AttributesHashMap> delta_metrics = nullptr;
{
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(hashmap_lock_);
std::lock_guard<std::mutex> guard(hashmap_lock_);
delta_metrics = std::move(delta_hash_map_);
delta_hash_map_ =
std::make_unique<AttributesHashMap>(aggregation_config_->cardinality_limit_);
Expand All @@ -148,7 +148,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora
const AggregationConfig *aggregation_config_;
std::unique_ptr<AttributesHashMap> cumulative_hash_map_;
std::unique_ptr<AttributesHashMap> delta_hash_map_;
opentelemetry::common::SpinLockMutex hashmap_lock_;
std::mutex hashmap_lock_;
#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW
ExemplarFilterType exemplar_filter_type_;
nostd::shared_ptr<ExemplarReservoir> exemplar_reservoir_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <unordered_map>

#include "opentelemetry/common/key_value_iterable.h"
#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/common/timestamp.h"
#include "opentelemetry/context/context.h"
#include "opentelemetry/nostd/function_ref.h"
Expand Down Expand Up @@ -226,7 +225,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage
InstrumentValueType value_type_;
MetricAttributes attributes_;
// Protected by lock_.
opentelemetry::common::SpinLockMutex lock_;
std::mutex lock_;
std::unique_ptr<Aggregation> current_;
bool dirty_ = false;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@

#include <list>
#include <memory>
#include <mutex>
#include <unordered_map>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/common/timestamp.h"
#include "opentelemetry/nostd/function_ref.h"
#include "opentelemetry/nostd/span.h"
Expand Down Expand Up @@ -54,7 +54,7 @@ class TemporalMetricStorage
std::unordered_map<CollectorHandle *, LastReportedMetrics> last_reported_metrics_;

// Lock while building metrics
mutable opentelemetry::common::SpinLockMutex lock_;
mutable std::mutex lock_;
const AggregationConfig *aggregation_config_;
opentelemetry::common::SystemTimestamp last_delta_collection_ts_;
bool has_last_delta_collection_ts_ = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <ostream>
#include <utility>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/nostd/variant.h"
#include "opentelemetry/sdk/common/global_log_handler.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
Expand Down Expand Up @@ -184,7 +183,7 @@ void Base2ExponentialHistogramAggregation::Aggregate(
double value,
const PointAttributes & /* attributes */) noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
point_data_.sum_ += value;
point_data_.count_++;

Expand Down Expand Up @@ -456,7 +455,7 @@ std::unique_ptr<Aggregation> Base2ExponentialHistogramAggregation::Diff(

PointType Base2ExponentialHistogramAggregation::ToPoint() const noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);

Base2ExponentialHistogramPointData copy;
copy.sum_ = point_data_.sum_;
Expand Down
9 changes: 4 additions & 5 deletions sdk/src/metrics/aggregation/histogram_aggregation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include <utility>
#include <vector>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/nostd/variant.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h"
Expand Down Expand Up @@ -60,7 +59,7 @@ LongHistogramAggregation::LongHistogramAggregation(const HistogramPointData &dat
void LongHistogramAggregation::Aggregate(int64_t value,
const PointAttributes & /* attributes */) noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
point_data_.count_ += 1;
point_data_.sum_ = nostd::get<int64_t>(point_data_.sum_) + value;
if (record_min_max_)
Expand Down Expand Up @@ -101,7 +100,7 @@ std::unique_ptr<Aggregation> LongHistogramAggregation::Diff(const Aggregation &n

PointType LongHistogramAggregation::ToPoint() const noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
return point_data_;
}

Expand Down Expand Up @@ -139,7 +138,7 @@ DoubleHistogramAggregation::DoubleHistogramAggregation(const HistogramPointData
void DoubleHistogramAggregation::Aggregate(double value,
const PointAttributes & /* attributes */) noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
point_data_.count_ += 1;
point_data_.sum_ = nostd::get<double>(point_data_.sum_) + value;
if (record_min_max_)
Expand Down Expand Up @@ -181,7 +180,7 @@ std::unique_ptr<Aggregation> DoubleHistogramAggregation::Diff(

PointType DoubleHistogramAggregation::ToPoint() const noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
return point_data_;
}

Expand Down
9 changes: 4 additions & 5 deletions sdk/src/metrics/aggregation/lastvalue_aggregation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include <memory>
#include <mutex>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/common/timestamp.h"
#include "opentelemetry/nostd/variant.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
Expand Down Expand Up @@ -34,7 +33,7 @@ LongLastValueAggregation::LongLastValueAggregation(const LastValuePointData &dat
void LongLastValueAggregation::Aggregate(int64_t value,
const PointAttributes & /* attributes */) noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
point_data_.is_lastvalue_valid_ = true;
point_data_.value_ = value;
point_data_.sample_ts_ = std::chrono::system_clock::now();
Expand Down Expand Up @@ -73,7 +72,7 @@ std::unique_ptr<Aggregation> LongLastValueAggregation::Diff(const Aggregation &n

PointType LongLastValueAggregation::ToPoint() const noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
return point_data_;
}

Expand All @@ -90,7 +89,7 @@ DoubleLastValueAggregation::DoubleLastValueAggregation(const LastValuePointData
void DoubleLastValueAggregation::Aggregate(double value,
const PointAttributes & /* attributes */) noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
point_data_.is_lastvalue_valid_ = true;
point_data_.value_ = value;
point_data_.sample_ts_ = std::chrono::system_clock::now();
Expand Down Expand Up @@ -130,7 +129,7 @@ std::unique_ptr<Aggregation> DoubleLastValueAggregation::Diff(

PointType DoubleLastValueAggregation::ToPoint() const noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
return point_data_;
}
} // namespace metrics
Expand Down
9 changes: 4 additions & 5 deletions sdk/src/metrics/aggregation/sum_aggregation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include <mutex>
#include <ostream>

#include "opentelemetry/common/spin_lock_mutex.h"
#include "opentelemetry/nostd/variant.h"
#include "opentelemetry/sdk/common/global_log_handler.h"
#include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
Expand Down Expand Up @@ -39,7 +38,7 @@ void LongSumAggregation::Aggregate(int64_t value, const PointAttributes & /* att
<< value);
return;
}
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
point_data_.value_ = nostd::get<int64_t>(point_data_.value_) + value;
}

Expand Down Expand Up @@ -97,7 +96,7 @@ std::unique_ptr<Aggregation> LongSumAggregation::Diff(const Aggregation &next) c

PointType LongSumAggregation::ToPoint() const noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
return point_data_;
}

Expand All @@ -120,7 +119,7 @@ void DoubleSumAggregation::Aggregate(double value,
<< value);
return;
}
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
point_data_.value_ = nostd::get<double>(point_data_.value_) + value;
}

Expand Down Expand Up @@ -178,7 +177,7 @@ std::unique_ptr<Aggregation> DoubleSumAggregation::Diff(const Aggregation &next)

PointType DoubleSumAggregation::ToPoint() const noexcept
{
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(lock_);
const std::lock_guard<std::mutex> locked(lock_);
return point_data_;
}

Expand Down
11 changes: 5 additions & 6 deletions sdk/src/metrics/state/sync_metric_storage.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
# include <unordered_set>
# include <vector>

# include "opentelemetry/common/spin_lock_mutex.h"
# include "opentelemetry/sdk/common/global_log_handler.h"
# include "opentelemetry/sdk/metrics/aggregation/aggregation.h"
# include "opentelemetry/sdk/metrics/data/exemplar_data.h"
Expand Down Expand Up @@ -66,7 +65,7 @@ bool SyncMetricStorage::Collect(CollectorHandle *collector,
bool can_erase = false;
if (it->second.use_count() == 1)
{
std::lock_guard<opentelemetry::common::SpinLockMutex> g(it->second->lock_);
std::lock_guard<std::mutex> g(it->second->lock_);
can_erase = !it->second->dirty_;
}
if (can_erase)
Expand Down Expand Up @@ -103,7 +102,7 @@ bool SyncMetricStorage::Collect(CollectorHandle *collector,
std::unique_ptr<Aggregation> rotated;
MetricAttributes attrs_copy;
{
std::lock_guard<opentelemetry::common::SpinLockMutex> g(entry->lock_);
std::lock_guard<std::mutex> g(entry->lock_);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The above comments still says spinlock while it has chanegd to std::mutex.

if (!entry->dirty_)
{
continue;
Expand Down Expand Up @@ -160,7 +159,7 @@ bool SyncMetricStorage::Collect(CollectorHandle *collector,
// satisfy the documented invariant on dirty_.
bool entry_dirty = false;
{
std::lock_guard<opentelemetry::common::SpinLockMutex> g(it->second->lock_);
std::lock_guard<std::mutex> g(it->second->lock_);
entry_dirty = it->second->dirty_;
}
if (entry_dirty)
Expand Down Expand Up @@ -228,7 +227,7 @@ void SyncMetricStorage::BoundEntry::RecordLong(int64_t value) noexcept
"is not long");
return;
}
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(lock_);
std::lock_guard<std::mutex> guard(lock_);
current_->Aggregate(value);
dirty_ = true;
}
Expand All @@ -242,7 +241,7 @@ void SyncMetricStorage::BoundEntry::RecordDouble(double value) noexcept
"is not double");
return;
}
std::lock_guard<opentelemetry::common::SpinLockMutex> guard(lock_);
std::lock_guard<std::mutex> guard(lock_);
current_->Aggregate(value);
dirty_ = true;
}
Expand Down
Loading
Loading