Skip to content

Commit

Permalink
[BUILD] Remove defining NOMINMAX from api (#2420)
Browse files Browse the repository at this point in the history
  • Loading branch information
ThomsonTan authored Dec 2, 2023
1 parent 25343e6 commit abad83d
Show file tree
Hide file tree
Showing 20 changed files with 35 additions and 42 deletions.
1 change: 0 additions & 1 deletion api/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ if(WITH_NO_GETENV)
endif()

if(WIN32)
target_compile_definitions(opentelemetry_api INTERFACE NOMINMAX)
if(WITH_ETW)
target_compile_definitions(opentelemetry_api INTERFACE HAVE_MSGPACK)
endif()
Expand Down
3 changes: 0 additions & 3 deletions api/include/opentelemetry/common/spin_lock_mutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@
#include "opentelemetry/version.h"

#if defined(_MSC_VER)
# ifndef NOMINMAX
# define NOMINMAX
# endif
# define _WINSOCKAPI_ // stops including winsock.h
# include <windows.h>
#elif defined(__i386__) || defined(__x86_64__)
Expand Down
6 changes: 3 additions & 3 deletions api/include/opentelemetry/common/timestamp.h
Original file line number Diff line number Diff line change
Expand Up @@ -178,21 +178,21 @@ class DurationUtil
std::chrono::duration<Rep, Period> indefinite_value) noexcept
{
// Do not call now() when this duration is max value, now() may have a expensive cost.
if (timeout == std::chrono::duration<Rep, Period>::max())
if (timeout == (std::chrono::duration<Rep, Period>::max)())
{
return indefinite_value;
}

// std::future<T>::wait_for, std::this_thread::sleep_for, and std::condition_variable::wait_for
// may use steady_clock or system_clock.We need make sure now() + timeout do not overflow.
auto max_timeout = std::chrono::duration_cast<std::chrono::duration<Rep, Period>>(
std::chrono::steady_clock::time_point::max() - std::chrono::steady_clock::now());
(std::chrono::steady_clock::time_point::max)() - std::chrono::steady_clock::now());
if (timeout >= max_timeout)
{
return indefinite_value;
}
max_timeout = std::chrono::duration_cast<std::chrono::duration<Rep, Period>>(
std::chrono::system_clock::time_point::max() - std::chrono::system_clock::now());
(std::chrono::system_clock::time_point::max)() - std::chrono::system_clock::now());
if (timeout >= max_timeout)
{
return indefinite_value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@
#include "opentelemetry/plugin/hook.h"
#include "opentelemetry/version.h"

#ifndef NOMINMAX
# define NOMINMAX
#endif
#include <Windows.h>

#include <WinBase.h>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ class OStreamLogRecordExporter final : public opentelemetry::sdk::logs::LogRecor
* @return return true when all data are exported, and false when timeout
*/
bool ForceFlush(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept override;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override;

/**
* Marks the OStream Log Exporter as shut down.
*/
bool Shutdown(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept override;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override;

private:
// The OStream to send the logs to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ class OStreamSpanExporter final : public opentelemetry::sdk::trace::SpanExporter
* @return return true when all data are exported, and false when timeout
*/
bool ForceFlush(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept override;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override;

bool Shutdown(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept override;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override;

private:
std::ostream &sout_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ class ZipkinExporter final : public opentelemetry::sdk::trace::SpanExporter
* @return return true when all data are exported, and false when timeout
*/
bool ForceFlush(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept override;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override;

/**
* Shut down the exporter.
* @param timeout an optional timeout, default to max.
*/
bool Shutdown(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept override;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override;

private:
void InitializeLocalEndpoint();
Expand Down
2 changes: 1 addition & 1 deletion sdk/include/opentelemetry/sdk/logs/exporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class OPENTELEMETRY_EXPORT LogRecordExporter
* @return true if the exporter shutdown succeeded, false otherwise
*/
virtual bool Shutdown(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept = 0;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept = 0;
};
} // namespace logs
} // namespace sdk
Expand Down
2 changes: 1 addition & 1 deletion sdk/include/opentelemetry/sdk/logs/logger_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class LoggerContext
/**
* Shutdown the log processor associated with this tracer provider.
*/
bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept;
bool Shutdown(std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept;

private:
// order of declaration is important here - resource object should be destroyed after processor.
Expand Down
4 changes: 2 additions & 2 deletions sdk/include/opentelemetry/sdk/logs/processor.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class LogRecordProcessor
* @return a result code indicating whether it succeeded, failed or timed out
*/
virtual bool ForceFlush(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept = 0;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept = 0;

/**
* Shuts down the processor and does any cleanup required.
Expand All @@ -56,7 +56,7 @@ class LogRecordProcessor
* @return true if the shutdown succeeded, false otherwise
*/
virtual bool Shutdown(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept = 0;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept = 0;
};
} // namespace logs
} // namespace sdk
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ class SimpleLogRecordProcessor : public LogRecordProcessor
void OnEmit(std::unique_ptr<Recordable> &&record) noexcept override;

bool ForceFlush(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept override;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override;

bool Shutdown(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept override;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept override;

bool IsShutdown() const noexcept;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ void HistogramMerge(HistogramPointData &current,
merge.record_min_max_ = current.record_min_max_ && delta.record_min_max_;
if (merge.record_min_max_)
{
merge.min_ = std::min(nostd::get<T>(current.min_), nostd::get<T>(delta.min_));
merge.max_ = std::max(nostd::get<T>(current.max_), nostd::get<T>(delta.max_));
merge.min_ = (std::min)(nostd::get<T>(current.min_), nostd::get<T>(delta.min_));
merge.max_ = (std::max)(nostd::get<T>(current.max_), nostd::get<T>(delta.max_));
}
}

Expand Down
4 changes: 2 additions & 2 deletions sdk/include/opentelemetry/sdk/metrics/metric_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ class MetricReader
/**
* Shutdown the metric reader.
*/
bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept;
bool Shutdown(std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept;

/**
* Force flush the metric read by the reader.
*/
bool ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept;
bool ForceFlush(std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept;

/**
* Return the status of Metric reader.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ class MetricCollector : public MetricProducer, public CollectorHandle
*/
bool Collect(nostd::function_ref<bool(ResourceMetrics &metric_data)> callback) noexcept override;

bool ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept;
bool ForceFlush(std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept;

bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept;
bool Shutdown(std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept;

private:
MeterContext *meter_context_;
Expand Down
2 changes: 1 addition & 1 deletion sdk/include/opentelemetry/sdk/trace/exporter.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class OPENTELEMETRY_EXPORT SpanExporter
* @return return the status of the operation.
*/
virtual bool Shutdown(
std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept = 0;
std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept = 0;
};
} // namespace trace
} // namespace sdk
Expand Down
2 changes: 1 addition & 1 deletion sdk/src/logs/batch_log_record_processor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ bool BatchLogRecordProcessor::ForceFlush(std::chrono::microseconds timeout) noex
std::chrono::duration_cast<std::chrono::steady_clock::duration>(timeout);
if (timeout_steady <= std::chrono::steady_clock::duration::zero())
{
timeout_steady = std::chrono::steady_clock::duration::max();
timeout_steady = (std::chrono::steady_clock::duration::max)();
}

bool result = false;
Expand Down
16 changes: 8 additions & 8 deletions sdk/src/metrics/aggregation/histogram_aggregation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ LongHistogramAggregation::LongHistogramAggregation(const AggregationConfig *aggr
point_data_.sum_ = (int64_t)0;
point_data_.count_ = 0;
point_data_.record_min_max_ = record_min_max_;
point_data_.min_ = std::numeric_limits<int64_t>::max();
point_data_.max_ = std::numeric_limits<int64_t>::min();
point_data_.min_ = (std::numeric_limits<int64_t>::max)();
point_data_.max_ = (std::numeric_limits<int64_t>::min)();
}

LongHistogramAggregation::LongHistogramAggregation(HistogramPointData &&data)
Expand All @@ -58,8 +58,8 @@ void LongHistogramAggregation::Aggregate(int64_t value,
point_data_.sum_ = nostd::get<int64_t>(point_data_.sum_) + value;
if (record_min_max_)
{
point_data_.min_ = std::min(nostd::get<int64_t>(point_data_.min_), value);
point_data_.max_ = std::max(nostd::get<int64_t>(point_data_.max_), value);
point_data_.min_ = (std::min)(nostd::get<int64_t>(point_data_.min_), value);
point_data_.max_ = (std::max)(nostd::get<int64_t>(point_data_.max_), value);
}
size_t index = BucketBinarySearch(value, point_data_.boundaries_);
point_data_.counts_[index] += 1;
Expand Down Expand Up @@ -118,8 +118,8 @@ DoubleHistogramAggregation::DoubleHistogramAggregation(const AggregationConfig *
point_data_.sum_ = 0.0;
point_data_.count_ = 0;
point_data_.record_min_max_ = record_min_max_;
point_data_.min_ = std::numeric_limits<double>::max();
point_data_.max_ = std::numeric_limits<double>::min();
point_data_.min_ = (std::numeric_limits<double>::max)();
point_data_.max_ = (std::numeric_limits<double>::min)();
}

DoubleHistogramAggregation::DoubleHistogramAggregation(HistogramPointData &&data)
Expand All @@ -138,8 +138,8 @@ void DoubleHistogramAggregation::Aggregate(double value,
point_data_.sum_ = nostd::get<double>(point_data_.sum_) + value;
if (record_min_max_)
{
point_data_.min_ = std::min(nostd::get<double>(point_data_.min_), value);
point_data_.max_ = std::max(nostd::get<double>(point_data_.max_), value);
point_data_.min_ = (std::min)(nostd::get<double>(point_data_.min_), value);
point_data_.max_ = (std::max)(nostd::get<double>(point_data_.max_), value);
}
size_t index = BucketBinarySearch(value, point_data_.boundaries_);
point_data_.counts_[index] += 1;
Expand Down
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 @@ -133,7 +133,7 @@ bool PeriodicExportingMetricReader::OnForceFlush(std::chrono::microseconds timeo
std::chrono::duration_cast<std::chrono::steady_clock::duration>(wait_timeout);
if (timeout_steady <= std::chrono::steady_clock::duration::zero())
{
timeout_steady = std::chrono::steady_clock::duration::max();
timeout_steady = (std::chrono::steady_clock::duration::max)();
}

bool result = false;
Expand Down
4 changes: 2 additions & 2 deletions sdk/src/metrics/meter_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,15 @@ bool MeterContext::ForceFlush(std::chrono::microseconds timeout) noexcept
// 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();
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();
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)
Expand Down
2 changes: 1 addition & 1 deletion sdk/src/trace/batch_span_processor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ bool BatchSpanProcessor::ForceFlush(std::chrono::microseconds timeout) noexcept
std::chrono::duration_cast<std::chrono::steady_clock::duration>(timeout);
if (timeout_steady <= std::chrono::steady_clock::duration::zero())
{
timeout_steady = std::chrono::steady_clock::duration::max();
timeout_steady = (std::chrono::steady_clock::duration::max)();
}

bool result = false;
Expand Down

2 comments on commit abad83d

@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: abad83d Previous: 25343e6 Ratio
BM_LockFreeBuffer/1 6079368.591308594 ns/iter 1430428.5049438477 ns/iter 4.25
BM_LockFreeBuffer/2 10232172.012329102 ns/iter 1083742.6532492705 ns/iter 9.44
BM_LockFreeBuffer/4 11619579.792022705 ns/iter 1238191.0481272403 ns/iter 9.38

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

@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 api Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: abad83d Previous: 25343e6 Ratio
BM_ProcYieldSpinLockThrashing/4/process_time/real_time 2.2034017663252983 ms/iter 0.7713864548037751 ms/iter 2.86
BM_NaiveSpinLockThrashing/2/process_time/real_time 0.3303087674654447 ms/iter 0.16491268282738802 ms/iter 2.00
BM_NaiveSpinLockThrashing/4/process_time/real_time 2.3708692411097085 ms/iter 0.6890296936035156 ms/iter 3.44

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

Please sign in to comment.