lidavidm commented on code in PR #12702:
URL: https://github.com/apache/arrow/pull/12702#discussion_r854671984
##########
cpp/src/arrow/util/tracing_internal.h:
##########
@@ -114,23 +116,44 @@ class SpanImpl {
opentelemetry::trace::StartSpanOptions SpanOptionsWithParent(
const util::tracing::Span& parent_span);
-#define START_SPAN(target_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
-
::arrow::internal::tracing::GetTracer()->StartSpan(__VA_ARGS__)}) \
- .span)
-
-#define START_SPAN_WITH_PARENT(target_span, parent_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
- ::arrow::internal::tracing::GetTracer()->StartSpan(
\
- __VA_ARGS__,
\
-
::arrow::internal::tracing::SpanOptionsWithParent(parent_span))}) \
- .span)
+#define CONFIGURE_TARGET_SPAN(target_span, ...) \
Review Comment:
I don't think we need to rearrange all these macros, right? You can keep the
original START_SPAN macros and just have START_COMPUTE_SPAN be START_SPAN + a
call to SetAttribute. Spans are mutable, there's no requirement that you fully
configure them before using them.
##########
cpp/src/arrow/util/tracing_internal.h:
##########
@@ -114,23 +116,44 @@ class SpanImpl {
opentelemetry::trace::StartSpanOptions SpanOptionsWithParent(
const util::tracing::Span& parent_span);
-#define START_SPAN(target_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
-
::arrow::internal::tracing::GetTracer()->StartSpan(__VA_ARGS__)}) \
- .span)
-
-#define START_SPAN_WITH_PARENT(target_span, parent_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
- ::arrow::internal::tracing::GetTracer()->StartSpan(
\
- __VA_ARGS__,
\
-
::arrow::internal::tracing::SpanOptionsWithParent(parent_span))}) \
- .span)
+#define CONFIGURE_TARGET_SPAN(target_span, ...) \
+ target_span \
+ .Set(::arrow::util::tracing::Span::Impl{ \
+ ::arrow::internal::tracing::GetTracer()->StartSpan(__VA_ARGS__)}) \
+ .span
+
+#define START_COMPUTE_SPAN(target_span, ...)
\
+ auto modified_span = CONFIGURE_TARGET_SPAN(target_span, __VA_ARGS__);
\
+ modified_span->SetAttribute("memory_pool_bytes",
\
Review Comment:
Note how the official ones are semi-namespaced:
https://github.com/open-telemetry/opentelemetry-cpp/blob/4f5505f9cb45326299067cfbd53db21198970aa8/sdk/include/opentelemetry/sdk/resource/experimental_semantic_conventions.h
##########
cpp/src/arrow/util/tracing_internal.h:
##########
@@ -114,23 +116,44 @@ class SpanImpl {
opentelemetry::trace::StartSpanOptions SpanOptionsWithParent(
const util::tracing::Span& parent_span);
-#define START_SPAN(target_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
-
::arrow::internal::tracing::GetTracer()->StartSpan(__VA_ARGS__)}) \
- .span)
-
-#define START_SPAN_WITH_PARENT(target_span, parent_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
- ::arrow::internal::tracing::GetTracer()->StartSpan(
\
- __VA_ARGS__,
\
-
::arrow::internal::tracing::SpanOptionsWithParent(parent_span))}) \
- .span)
+#define CONFIGURE_TARGET_SPAN(target_span, ...) \
Review Comment:
Or actually, maybe just have START_SPAN and CONFIGURE_COMPUTE_SPAN or
something. That way we have composable macros instead of having to define
duplicate macros for every scenario.
##########
cpp/src/arrow/util/tracing_internal.h:
##########
@@ -114,23 +116,44 @@ class SpanImpl {
opentelemetry::trace::StartSpanOptions SpanOptionsWithParent(
const util::tracing::Span& parent_span);
-#define START_SPAN(target_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
-
::arrow::internal::tracing::GetTracer()->StartSpan(__VA_ARGS__)}) \
- .span)
-
-#define START_SPAN_WITH_PARENT(target_span, parent_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
- ::arrow::internal::tracing::GetTracer()->StartSpan(
\
- __VA_ARGS__,
\
-
::arrow::internal::tracing::SpanOptionsWithParent(parent_span))}) \
- .span)
+#define CONFIGURE_TARGET_SPAN(target_span, ...) \
+ target_span \
+ .Set(::arrow::util::tracing::Span::Impl{ \
+ ::arrow::internal::tracing::GetTracer()->StartSpan(__VA_ARGS__)}) \
+ .span
+
+#define START_COMPUTE_SPAN(target_span, ...)
\
+ auto modified_span = CONFIGURE_TARGET_SPAN(target_span, __VA_ARGS__);
\
+ modified_span->SetAttribute("memory_pool_bytes",
\
Review Comment:
nit, but should we perhaps namespace this attribute name?
"arrow.memory_pool_bytes" or something
##########
cpp/src/arrow/util/tracing_internal.h:
##########
@@ -106,6 +106,8 @@ AsyncGenerator<T>
PropagateSpanThroughAsyncGenerator(AsyncGenerator<T> wrapped)
return PropagateSpanThroughAsyncGenerator(std::move(wrapped),
std::move(span));
}
+#define GET_MEMORY_POOL_INFO
Review Comment:
What's this define for?
##########
cpp/src/arrow/util/tracing_internal.h:
##########
@@ -114,23 +116,44 @@ class SpanImpl {
opentelemetry::trace::StartSpanOptions SpanOptionsWithParent(
const util::tracing::Span& parent_span);
-#define START_SPAN(target_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
-
::arrow::internal::tracing::GetTracer()->StartSpan(__VA_ARGS__)}) \
- .span)
-
-#define START_SPAN_WITH_PARENT(target_span, parent_span, ...)
\
- auto opentelemetry_scope##__LINE__ =
\
- ::arrow::internal::tracing::GetTracer()->WithActiveSpan(
\
- target_span
\
- .Set(::arrow::util::tracing::Span::Impl{
\
- ::arrow::internal::tracing::GetTracer()->StartSpan(
\
- __VA_ARGS__,
\
-
::arrow::internal::tracing::SpanOptionsWithParent(parent_span))}) \
- .span)
+#define CONFIGURE_TARGET_SPAN(target_span, ...) \
Review Comment:
Mostly I don't want to have a whole bunch of macros for little things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
