Arnab Karmakar 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 2: (7 comments) Thanks for the 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: public: > Use nostd::string_view instead of const std::string& Done 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: HttpHeaderCarrier carrier(traceparent, tracestate); : context::Context ctx; > I think these class members can be defined as type nostd::string_view since Done http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-propagation.cc@34 PS1, Line 34: if (key == trace::propagation::kTraceParent) { : return traceparent_; : } : if (key == trace::propagation::kTraceState) { : return tracestate_; : } : return ""; : } : : void HttpHeaderCarrier::Set(nostd::string_view key, : nostd::string_view value) noexcept {} : : trace::SpanContext ExtractSpanContextFromHttpHeaders(nostd::string_view traceparent, : nostd::string_view tracestate) { : if (traceparent.empty()) { : return trace::SpanContext::GetInvalid(); : } : : trace::propagation::HttpTraceContext propagator; : HttpHeaderCarrier carrier(traceparent, tracestate); : context::Context ctx; : > Class definition should be in the header file with function implementation Done 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: > Is there an easy way to assert that the tracestate was correctly extracted? Done http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/observe/otel-test.cc@386 PS1, Line 386: : EXPECT_EQ(tracestate, ctx.trace_state()->ToHeader()); > Please move each of these lines into their own individual tests. Done 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: std::shared_ptr<ImpalaServer::SessionState> session() const { return session_; } : : /// Returns the W3C Trace Context traceparent header from the hs2-http request that : /// submitted this query, if any. : std::string_view http_traceparent() const { return http_traceparent_; } : : /// Returns the W3C Trace Context tracestate header from the hs2-http r > Can these functions both return std::string_view instead of const std::stri Done http://gerrit.cloudera.org:8080/#/c/24706/1/be/src/service/client-request-state.h@640 PS1, Line 640: std::shared_ptr<ImpalaServer::SessionState> session_; : : /// W3C Trace Context headers > I don't know how long the connection context returned by ThriftServer::GetT I think the ConnectionContext outlives a single HTTP request (it is per-connection), and http_traceparent / http_tracestate are overwritten on each hs2-http RPC on that connection. ClientRequestState also outlives the submitting ExecuteStatement RPC. Did not change the members to string_view. -- 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: 2 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Mon, 24 Aug 2026 07:36:11 +0000 Gerrit-HasComments: Yes
