Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24706 )
Change subject: IMPALA-14752: Support W3C trace context propagation for hs2-http ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-propagation.h File be/src/observe/otel-propagation.h: http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-propagation.h@29 PS1, Line 29: const std::string& traceparent, const std::string& tracestate); Use nostd::string_view instead of const std::string& http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-propagation.cc File be/src/observe/otel-propagation.cc: http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-propagation.cc@53 PS1, Line 53: const string& traceparent_; : const string& tracestate_; I think these class members can be defined as type nostd::string_view since the lifetime of this class is limited to the duration of the ExtractSpanContextFromHttpHeaders() function. http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-propagation.cc@34 PS1, Line 34: // TextMapCarrier implementation that reads traceparent and tracestate header values. : class HttpHeaderCarrier : public context::propagation::TextMapCarrier { : public: : HttpHeaderCarrier(const string& traceparent, const string& tracestate) : : traceparent_(traceparent), tracestate_(tracestate) {} : : nostd::string_view Get(nostd::string_view key) const noexcept override { : if (key == trace::propagation::kTraceParent) { : return traceparent_; : } : if (key == trace::propagation::kTraceState) { : return tracestate_; : } : return ""; : } : : void Set(nostd::string_view key, nostd::string_view value) noexcept override {} : : private: : const string& traceparent_; : const string& tracestate_; : }; Class definition should be in the header file with function implementation (of the Get() function) in the .cc file. http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-test.cc File be/src/observe/otel-test.cc: http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-test.cc@371 PS1, Line 371: SpanContext ctx = ExtractSpanContextFromHttpHeaders(traceparent, ""); Is there an easy way to assert that the tracestate was correctly extracted? If so, please add assertions for that too even though Impala does not use trace state right now. http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-test.cc@386 PS1, Line 386: EXPECT_FALSE(ExtractSpanContextFromHttpHeaders("", "").IsValid()); : EXPECT_FALSE(ExtractSpanContextFromHttpHeaders("invalid", "").IsValid()); Please move each of these lines into their own individual tests. http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/service/client-request-state.h@248 PS1, Line 248: /// Returns the W3C Trace Context traceparent header from the hs2-http request that : /// submitted this query, if any. : const std::string& http_traceparent() const { return http_traceparent_; } : : /// Returns the W3C Trace Context tracestate header from the hs2-http request that : /// submitted this query, if any. : const std::string& http_tracestate() const { return http_tracestate_; } Can these functions both return std::string_view instead of const std::string& http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/service/client-request-state.h@640 PS1, Line 640: /// W3C Trace Context headers from the hs2-http request that submitted this query. : std::string http_traceparent_; : std::string http_tracestate_; I don't know how long the connection context returned by ThriftServer::GetThreadConnectionContext() lives, but if it is for the duration of the request, these variables could be std::string_view instead of std::string. -- To view, visit http://gerrit.cloudera.org:8080/24706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieac548f4f5f613ba116d1a79d95e31b7a1a37b97 Gerrit-Change-Number: 24706 Gerrit-PatchSet: 1 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Thu, 20 Aug 2026 22:32:23 +0000 Gerrit-HasComments: Yes
