Skip to content

Commit

Permalink
Fix overflow in timeout logic (#3046)
Browse files Browse the repository at this point in the history
  • Loading branch information
punya authored Sep 3, 2024
1 parent a920898 commit f02d70b
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 23 deletions.
2 changes: 1 addition & 1 deletion sdk/src/metrics/export/periodic_exporting_metric_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ bool PeriodicExportingMetricReader::OnForceFlush(std::chrono::microseconds timeo
// - If original `timeout` is `zero`, use that in exporter::forceflush
// - Else if remaining `timeout_steady` more than zero, use that in exporter::forceflush
// - Else don't invoke exporter::forceflush ( as remaining time is zero or less)
if (timeout <= std::chrono::steady_clock::duration::zero())
if (timeout <= std::chrono::milliseconds::duration::zero())
{
result =
exporter_->ForceFlush(std::chrono::duration_cast<std::chrono::microseconds>(timeout));
Expand Down
34 changes: 12 additions & 22 deletions sdk/src/metrics/meter_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -168,46 +168,36 @@ bool MeterContext::ForceFlush(std::chrono::microseconds timeout) noexcept
bool result = true;
// Simultaneous flush not allowed.
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(forceflush_lock_);
// Convert to nanos to prevent overflow
auto timeout_ns = (std::chrono::nanoseconds::max)();
if (std::chrono::duration_cast<std::chrono::microseconds>(timeout_ns) > timeout)
{
timeout_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(timeout);
}

auto current_time = std::chrono::system_clock::now();
std::chrono::system_clock::time_point expire_time;
auto overflow_checker = (std::chrono::system_clock::time_point::max)();

// check if the expected expire time doesn't overflow.
if (overflow_checker - current_time > timeout_ns)
auto time_remaining = (std::chrono::steady_clock::duration::max)();
if (std::chrono::duration_cast<std::chrono::microseconds>(time_remaining) > timeout)
{
expire_time =
current_time + std::chrono::duration_cast<std::chrono::system_clock::duration>(timeout_ns);
time_remaining = timeout;
}
else

auto current_time = std::chrono::steady_clock::now();
auto expire_time = (std::chrono::steady_clock::time_point::max)();
if (expire_time - current_time > time_remaining)
{
// overflow happens, reset expire time to max.
expire_time = overflow_checker;
expire_time = current_time + time_remaining;
}

for (auto &collector : collectors_)
{
if (!std::static_pointer_cast<MetricCollector>(collector)->ForceFlush(
std::chrono::duration_cast<std::chrono::microseconds>(timeout_ns)))
std::chrono::duration_cast<std::chrono::microseconds>(time_remaining)))
{
result = false;
}

current_time = std::chrono::system_clock::now();

current_time = std::chrono::steady_clock::now();
if (expire_time >= current_time)
{
timeout_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(expire_time - current_time);
time_remaining = expire_time - current_time;
}
else
{
timeout_ns = std::chrono::nanoseconds::zero();
time_remaining = std::chrono::steady_clock::duration::zero();
}
}
if (!result)
Expand Down

1 comment on commit f02d70b

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp sdk Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: f02d70b Previous: a920898 Ratio
BM_BaselineBuffer/1 12306020.259857178 ns/iter 6127998.59046936 ns/iter 2.01

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.