ARROW-14958: [C++][Python][FlightRPC] Implement Flight middleware for OpenTelemetry propagation #11920

lidavidm · 2021-12-09T18:59:09Z

Adds a client middleware that sends span/trace ID to the server, and a server middleware that gets the span/trace ID and starts a child span.

The middleware are available in builds without OpenTelemetry, they simply do nothing.

github-actions · 2021-12-09T18:59:28Z

https://issues.apache.org/jira/browse/ARROW-14958

lidavidm · 2021-12-09T18:59:54Z

Example output, from the unit test:

(The unit test will not normally print this, I just modified it to double-check the results.)

{
  name          : DoAction
  trace_id      : a9d2a9ecd0aec3bb44bd10ad940a31b7
  span_id       : 7b025703fe739005
  tracestate    : 
  parent_span_id: 0000000000000000
  start         : 1639076215603966576
  duration      : 739917
  description   : 
  span kind     : Server
  status        : Ok
  attributes    : 
	thread_id: 140075521935104
	rpc.grpc.status_code: 0
	rpc.method: DoAction
	rpc.service: arrow.flight.protocol.FlightService
	rpc.system: grpc
  events        : 
  links         : 
  resources     : 
	service.name: unknown_service
	telemetry.sdk.version: 1.1.0
	telemetry.sdk.name: opentelemetry
	telemetry.sdk.language: cpp
  instr-lib     : arrow
}
{
  name          : DoAction
  trace_id      : a605cd6ac77d28ea4fc94062bb0ba455
  span_id       : f599a779803fdf67
  tracestate    : 
  parent_span_id: a9d7e9435894a240
  start         : 1639076215610106244
  duration      : 236620
  description   : 
  span kind     : Server
  status        : Ok
  attributes    : 
	thread_id: 140075513542400
	rpc.grpc.status_code: 0
	rpc.method: DoAction
	rpc.service: arrow.flight.protocol.FlightService
	rpc.system: grpc
  events        : 
  links         : 
  resources     : 
	service.name: unknown_service
	telemetry.sdk.version: 1.1.0
	telemetry.sdk.name: opentelemetry
	telemetry.sdk.language: cpp
  instr-lib     : arrow
}
{
  name          : test
  trace_id      : a605cd6ac77d28ea4fc94062bb0ba455
  span_id       : a9d7e9435894a240
  tracestate    : 
  parent_span_id: 0000000000000000
  start         : 1639076215607131441
  duration      : 3385487
  description   : 
  span kind     : Internal
  status        : Unset
  attributes    : 
	thread_id: 140075633232128
  events        : 
  links         : 
  resources     : 
	service.name: unknown_service
	telemetry.sdk.version: 1.1.0
	telemetry.sdk.name: opentelemetry
	telemetry.sdk.language: cpp
  instr-lib     : arrow
}

lidavidm · 2021-12-09T21:02:26Z

CC @cpcloud, this is "automatic" instrumentation for Flight/OpenTelemetry. AFAIK, this isn't generally possible in gRPC/C++. Server interceptors have no way to pass data from the interceptor to the RPC handler. (OpenCensus support is achieved by hardcoding it into the library.) Also, IIRC thread locals (i.e. the OTel Context) are not viable because the library makes no guarantee about whether RPC handlers are run on the same thread as interceptors or not.

Flight works around this because the server interceptors don't use the gRPC interceptor framework; instead, the Flight RPC handlers hardcode calls to the interceptors before handing control to the Flight application. Hence, a Span started in a Flight interceptor will be active during the application's RPC handler.

The OpenTelemetry/gRPC example just hardcodes a call to OpenTelemetry within the RPC handler and does not try to implement more general instrumentation.

Quickly bump the version since it changes a few APIs we'll use (most notably for #11920). #11963 will also need updating, but the conda-forge packages need to be updated first. This does not include the fix needed for #12408, that will require another version bump. Closes #12516 from lidavidm/arrow-15789 Authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]>

pitrou · 2022-05-04T16:29:49Z

@lidavidm Does this PR need reviving or should it be closed?

lidavidm · 2022-05-04T16:30:22Z

I'll clean this up at some point. It mostly needs a suitable reviewer.

pitrou · 2022-05-30T14:36:22Z

It seems this needs rebasing and fixing conflicts :-(

pitrou · 2022-06-01T16:20:42Z

cpp/src/arrow/flight/client_tracing_middleware.cc

+    auto context = otel::context::RuntimeContext::GetCurrent();
+    auto propagator =
+        otel::context::propagation::GlobalTextMapPropagator::GetGlobalPropagator();
+    propagator->Inject(carrier, context);


Wouldn't this make a copy of carrier?

No, OTel uses a reference here: https://github.com/open-telemetry/opentelemetry-cpp/blob/9df5c4fef13e962a51768c098fb07a8b4421dd97/api/include/opentelemetry/context/propagation/text_map_propagator.h#L50

pitrou · 2022-06-01T16:21:33Z

cpp/src/arrow/flight/client_tracing_middleware.cc

+
+  void SendingHeaders(AddCallHeaders* outgoing_headers) override {
+    for (const auto& pair : carrier_.context_) {
+      outgoing_headers->AddHeader(pair.first, pair.second);


Hmm... if OTel is adding arbitrary key-value pairs, should these really be propagated as HTTP headers, or am I missing something?

It's designed to be transported in headers, it's just that the API doesn't tell you what they are. They're well defined in the spec (formats listed here: https://github.com/open-telemetry/opentelemetry-specification/blob/e2c2472985b17e37a25a7dc5aa0aa071e6683c98/specification/context/api-propagators.md#propagators-distribution)

Hmm, can you add a comment about that?

pitrou · 2022-06-01T16:23:33Z

cpp/src/arrow/flight/middleware.h

@@ -61,6 +60,10 @@ enum class FlightMethod : char {
  DoExchange = 9,
 };

+/// \brief Get a human-readable name for a Flight method.
+ARROW_FLIGHT_EXPORT
+std::string FlightMethodToString(FlightMethod method);


We already use the ToString convention in other places, so perhaps:

Suggested change

std::string FlightMethodToString(FlightMethod method);

std::string ToString(FlightMethod method);

pitrou · 2022-06-01T16:24:41Z

cpp/src/arrow/flight/server_tracing_middleware.h

+    TraceKey() = default;
+    TraceKey(std::string key, std::string value)
+        : key(std::move(key)), value(std::move(value)) {}


C++ should synthesize these constructors automatically?

Apparently this won't work until C++20 (https://stackoverflow.com/a/61205386/262727)

No, but you could try items_->push_back({std::string(key), std::string(value)})

pitrou · 2022-06-01T16:34:17Z

cpp/src/arrow/flight/server_tracing_middleware.cc

+    } else {
+      span_->SetStatus(otel::trace::StatusCode::kOk, "");
+      span_->SetAttribute(OTEL_GET_TRACE_ATTR(AttrRpcGrpcStatusCode), int32_t(0));
+    }


Newbie question, but will this automatically end the span?

It'll end when the Span goes out of scope, but I'll just manually end it here to be explicit.

pitrou · 2022-06-01T16:35:26Z

cpp/src/arrow/flight/server_tracing_middleware.h

+    TraceKey() = default;
+    TraceKey(std::string key, std::string value)
+        : key(std::move(key)), value(std::move(value)) {}


No, but you could try items_->push_back({std::string(key), std::string(value)})

pitrou · 2022-06-01T16:35:58Z

cpp/src/arrow/flight/types.h

+  Result() = default;
+  explicit Result(std::shared_ptr<Buffer> body) : body(std::move(body)) {}


Neither this should be necessary?

pitrou · 2022-06-01T16:36:35Z

cpp/src/arrow/flight/client_tracing_middleware.cc

+
+  void SendingHeaders(AddCallHeaders* outgoing_headers) override {
+    for (const auto& pair : carrier_.context_) {
+      outgoing_headers->AddHeader(pair.first, pair.second);


Hmm, can you add a comment about that?

pitrou · 2022-06-01T16:37:32Z

cpp/src/arrow/flight/flight_test.cc

+        reinterpret_cast<TracingServerMiddleware*>(call_context.GetMiddleware("tracing"));
+    if (!middleware) return Status::Invalid("Could not find middleware");
+#ifdef ARROW_WITH_OPENTELEMETRY
+    EXPECT_GT(middleware->GetTraceContext().size(), 0);


Can you comment a bit on what this does/tests?

pitrou · 2022-06-01T16:38:38Z

cpp/src/arrow/flight/flight_test.cc

+    opentelemetry::context::propagation::GlobalTextMapPropagator::SetGlobalPropagator(
+        opentelemetry::nostd::shared_ptr<
+            opentelemetry::context::propagation::TextMapPropagator>(
+            new opentelemetry::trace::propagation::HttpTraceContext()));


Is this the kind of setup that third party server code would have to write as well?

Yes, the application would do this as part of initializing OpenTelemetry, though generally the SDKs provide conveniences to configure this.

pitrou · 2022-06-01T16:45:19Z

python/pyarrow/tests/test_flight.py

+    def do_action(self, context, action):
+        trace_context = context.get_middleware("tracing").trace_context
+        # Don't turn this method into a generator since then it'll be
+        # lazily evaluated, and the trace context will be lost


If trace_context is a regular Python dict, how would it be lost?

Evaluating .trace_context is side-effectful and depends on implicit state maintained by OpenTelemetry, so if this is a generator it'll be evaluated after OpenTelemetry has already cleaned up the state.

I've updated the comment to be clearer about what goes on.

lidavidm · 2022-09-22T13:09:49Z

Rebased, will merge assuming no further comments as this has been sitting around for a long time

ursabot · 2022-09-26T16:05:57Z

Benchmark runs are scheduled for baseline = f941118 and contender = be30611. be30611 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.2% ⬆️0.0%] test-mac-arm
[Failed ⬇️1.37% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.14% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] be306118 ec2-t3-xlarge-us-east-2
[Finished] be306118 test-mac-arm
[Failed] be306118 ursa-i9-9960x
[Finished] be306118 ursa-thinkcentre-m75q
[Finished] f941118e ec2-t3-xlarge-us-east-2
[Failed] f941118e test-mac-arm
[Failed] f941118e ursa-i9-9960x
[Finished] f941118e ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

… OpenTelemetry propagation (apache#11920) Adds a client middleware that sends span/trace ID to the server, and a server middleware that gets the span/trace ID and starts a child span. The middleware are available in builds without OpenTelemetry, they simply do nothing. Authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]>

github-actions bot added Component: FlightRPC Component: C++ labels Dec 9, 2021

lidavidm force-pushed the arrow-14958 branch from 0b14ba8 to c72a5c7 Compare December 14, 2021 14:46

lidavidm mentioned this pull request Feb 25, 2022

ARROW-15789: [C++] Update OpenTelemetry to v1.2.0 #12516

Closed

lidavidm force-pushed the arrow-14958 branch from c72a5c7 to c19fe58 Compare February 28, 2022 14:20

lidavidm changed the title ~~ARROW-14958: [C++] Implement Flight middleware for OpenTelemetry propagation~~ ARROW-14958: [C++][Python] Implement Flight middleware for OpenTelemetry propagation Feb 28, 2022

github-actions bot added the Component: Python label Feb 28, 2022

lidavidm force-pushed the arrow-14958 branch from c19fe58 to 9b23aa0 Compare February 28, 2022 14:56

lidavidm changed the title ~~ARROW-14958: [C++][Python] Implement Flight middleware for OpenTelemetry propagation~~ ARROW-14958: [C++][Python][FlightRPC] Implement Flight middleware for OpenTelemetry propagation Mar 8, 2022

lidavidm force-pushed the arrow-14958 branch from ebe5dc2 to f29ada7 Compare March 8, 2022 14:24

lidavidm marked this pull request as draft May 4, 2022 16:30

lidavidm force-pushed the arrow-14958 branch 3 times, most recently from 16818dc to 38eec85 Compare May 24, 2022 17:27

lidavidm marked this pull request as ready for review May 24, 2022 21:03

lidavidm force-pushed the arrow-14958 branch 2 times, most recently from a7c8757 to dd6a25a Compare May 31, 2022 16:35

pitrou reviewed Jun 1, 2022

View reviewed changes

lidavidm force-pushed the arrow-14958 branch from dd6a25a to 31219a5 Compare June 1, 2022 16:32

pitrou reviewed Jun 1, 2022

View reviewed changes

lidavidm force-pushed the arrow-14958 branch from f4f464c to 16fd5ba Compare September 22, 2022 13:09

lidavidm force-pushed the arrow-14958 branch 2 times, most recently from 8c211f3 to 6a8660a Compare September 25, 2022 01:01

ARROW-14958: [C++][Python][FlightRPC] Add OpenTelemetry middleware

93ac104

lidavidm force-pushed the arrow-14958 branch from 6a8660a to 93ac104 Compare September 25, 2022 11:42

lidavidm merged commit be30611 into apache:master Sep 26, 2022

lidavidm deleted the arrow-14958 branch September 26, 2022 11:42

rxm7706 mentioned this pull request Dec 12, 2022

[RFC] Keep building for py37 conda-forge/arrow-cpp-feedstock#896

Closed

asfimport mentioned this pull request Sep 26, 2022

[C++][FlightRPC] Enable OpenTelemetry with Arrow Flight #30480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-14958: [C++][Python][FlightRPC] Implement Flight middleware for OpenTelemetry propagation #11920

ARROW-14958: [C++][Python][FlightRPC] Implement Flight middleware for OpenTelemetry propagation #11920

lidavidm commented Dec 9, 2021

github-actions bot commented Dec 9, 2021

lidavidm commented Dec 9, 2021

lidavidm commented Dec 9, 2021

pitrou commented May 4, 2022

lidavidm commented May 4, 2022

pitrou commented May 30, 2022

pitrou Jun 1, 2022

lidavidm Jun 1, 2022

pitrou Jun 1, 2022

lidavidm Jun 1, 2022

pitrou Jun 1, 2022

pitrou Jun 1, 2022

pitrou Jun 1, 2022

lidavidm Jun 1, 2022

pitrou Jun 1, 2022

pitrou Jun 1, 2022

lidavidm Jun 1, 2022

pitrou Jun 1, 2022

pitrou Jun 1, 2022

pitrou Jun 1, 2022

pitrou Jun 1, 2022

pitrou Jun 1, 2022

lidavidm Jun 1, 2022

pitrou Jun 1, 2022

lidavidm Jun 1, 2022

lidavidm Jun 1, 2022

lidavidm commented Sep 22, 2022

ursabot commented Sep 26, 2022

	std::string FlightMethodToString(FlightMethod method);
	std::string ToString(FlightMethod method);

		Result() = default;
		explicit Result(std::shared_ptr<Buffer> body) : body(std::move(body)) {}

ARROW-14958: [C++][Python][FlightRPC] Implement Flight middleware for OpenTelemetry propagation #11920

ARROW-14958: [C++][Python][FlightRPC] Implement Flight middleware for OpenTelemetry propagation #11920

Conversation

lidavidm commented Dec 9, 2021

github-actions bot commented Dec 9, 2021

lidavidm commented Dec 9, 2021

lidavidm commented Dec 9, 2021

pitrou commented May 4, 2022

lidavidm commented May 4, 2022

pitrou commented May 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Sep 22, 2022

ursabot commented Sep 26, 2022