westonpace commented on a change in pull request #12609:
URL: https://github.com/apache/arrow/pull/12609#discussion_r826520649
##########
File path: cpp/src/arrow/util/tracing_internal.h
##########
@@ -146,6 +210,19 @@ opentelemetry::trace::StartSpanOptions
SpanOptionsWithParent(
return st;
\
})
+#define GET_CURRENT_SPAN(span) \
+ auto span = ::arrow::internal::tracing::GetTracer()->GetCurrentSpan()
+
+#define SET_SPAN_SCOPE(scope, span) \
+ auto scope = ::arrow::internal::tracing::GetTracer()->WithActiveSpan(span)
+
+#define TIE_SPAN_TO_GENERATOR(generator) \
+ generator =
::arrow::internal::tracing::TieSpanToAsyncGenerator(generator)
+
+#define PROPAGATE_SPAN_TO_GENERATOR(generator) \
+ generator = \
+
::arrow::internal::tracing::PropagateSpanThroughAsyncGenerator(generator)
+
Review comment:
These macros seem more like aliases. I'm not sure they help more than
they obfuscate.
##########
File path: cpp/src/parquet/arrow/reader.cc
##########
@@ -1060,7 +1077,10 @@ class RowGroupGenerator {
}
auto ready = reader->parquet_reader()->WhenBuffered({row_group},
column_indices);
if (cpu_executor_) ready = cpu_executor_->TransferAlways(ready);
- return ready.Then([=]() -> ::arrow::Future<RecordBatchGenerator> {
+
+ GET_CURRENT_SPAN(span);
Review comment:
This is a confusing use of macros. At the very least I'd want to see
something like `GET_CURRENT_SPAN(&span)`
##########
File path: cpp/src/parquet/arrow/reader.cc
##########
@@ -270,6 +273,20 @@ class FileReaderImpl : public FileReader {
records_to_read +=
reader_->metadata()->RowGroup(row_group)->ColumnChunk(i)->num_values();
}
+#ifdef ARROW_WITH_OPENTELEMETRY
+ std::string column_name = reader_->metadata()->schema()->Column(i)->name();
+ std::string phys_type =
TypeToString(reader_->metadata()->schema()->Column(i)->physical_type());
+ auto span = ::arrow::internal::tracing::GetTracer()->GetCurrentSpan();
+ ::arrow::util::tracing::Span childspan;
+ ::arrow::util::tracing::Span parentspan;
+ parentspan.Set(::arrow::util::tracing::Span::Impl{span});
Review comment:
This is a confusing way to create a parent span. Why does
`GetCurrentSpan()` not return something we can use? Why do we need
`parentspan`?
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -388,10 +388,16 @@ Result<RecordBatchGenerator>
ParquetFileFormat::ScanBatchesAsync(
pre_filtered = true;
if (row_groups.empty()) return
MakeEmptyGenerator<std::shared_ptr<RecordBatch>>();
}
+#ifdef ARROW_WITH_OPENTELEMETRY
+ auto span = ::arrow::internal::tracing::GetTracer()->GetCurrentSpan();
+#endif
// Open the reader and pay the real IO cost.
auto make_generator =
[=](const std::shared_ptr<parquet::arrow::FileReader>& reader) mutable
-> Result<RecordBatchGenerator> {
+#ifdef ARROW_WITH_OPENTELEMETRY
+ auto scope = ::arrow::internal::tracing::GetTracer()->WithActiveSpan(span);
Review comment:
Is this needed? What call is accessing this scope? By the time we get
to line 427 I think this scope has been destroyed.
--
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]
