wwbmmm commented on code in PR #3140:
URL: https://github.com/apache/brpc/pull/3140#discussion_r2625497213
##########
src/brpc/controller.cpp:
##########
@@ -1387,8 +1390,18 @@ const Controller* Controller::sub(int index) const {
return NULL;
}
-uint64_t Controller::trace_id() const { return _span ? _span->trace_id() : 0; }
-uint64_t Controller::span_id() const { return _span ? _span->span_id() : 0; }
+uint64_t Controller::trace_id() const {
+ if (auto span = _span.lock()) {
+ return span->trace_id();
+ }
+ return 0;
+}
+uint64_t Controller::span_id() const {
Review Comment:
add a new line
##########
src/brpc/builtin/rpcz_service.cpp:
##########
@@ -372,22 +400,22 @@ static void PrintServerSpan(std::ostream& os, const
RpczSpan& span,
if (PrintAnnotationsAndRealTimeSpan(
os, span.start_send_real_us(),
- &last_time, extr, ARRAY_SIZE(extr))) {
+ &last_time, extr, ARRAY_SIZE(extr), &span)) {
if (entered_user_method) {
- os << " Leave " << WebEscape(span.full_method_name()) << std::endl;
+ os << " [ServerSpan " << SPAN_ID_STR << '=' << Hex(span.span_id())
<< "] Leave " << WebEscape(span.full_method_name()) << std::endl;
} else {
- os << " Responding" << std::endl;
+ os << " [ServerSpan " << SPAN_ID_STR << '=' << Hex(span.span_id())
<< "] Responding" << std::endl;
}
}
if (PrintAnnotationsAndRealTimeSpan(
os, span.sent_real_us(),
- &last_time, extr, ARRAY_SIZE(extr))) {
- os << " Responded(" << span.response_size() << ')' << std::endl;
+ &last_time, extr, ARRAY_SIZE(extr), &span)) {
Review Comment:
This format is a little bit tedious. How about `[Server SPAN#S1] Processing
the request in a new worker`?
##########
src/brpc/policy/baidu_rpc_protocol.cpp:
##########
@@ -642,12 +642,16 @@ void ProcessRpcRequest(InputMessageBase* msg_base) {
bthread_assign_data((void*)&server->thread_local_options());
}
- Span* span = NULL;
+ std::shared_ptr<Span> span;
if (IsTraceable(request_meta.has_trace_id())) {
span = Span::CreateServerSpan(
request_meta.trace_id(), request_meta.span_id(),
request_meta.parent_span_id(), msg->base_real_us());
+#if BRPC_SPAN_ENABLE_SHARED_PTR_API
accessor.set_span(span);
+#else
+ accessor.set_span(span.get());
Review Comment:
you can add an overloaded function `set_span(std::shared_ptr<Span> span)`,
so that you don't need to use `BRPC_SPAN_ENABLE_SHARED_PTR_API` everywhere.
##########
src/bthread/key.cpp:
##########
@@ -672,4 +694,19 @@ void* bthread_get_assigned_data() {
return bthread::tls_bls.assigned_data;
}
+int bthread_set_span_funcs(bthread_create_span_fn create_fn,
+ bthread_destroy_span_fn destroy_fn,
Review Comment:
I don't think `key.cpp` is responsible for span's lifecycle triggering
points.
`key.cpp` is for the implementation of `bthread_get/setspecific`, it manage
keytable_pool and keytable, which has no relationship to span.
What you need is to call the span functions in the bthread lifecycle
(create/end/...), the better place is in the `task_group.cpp`, you can get/set
bthread::tls_bls.rpcz_parent_span there.
And the implementation of `bthread_set_span_funcs` is better placed into
`bthread.cpp`, because most bthread interface functions are there.
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,97 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation
fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread",
base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ }
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
Review Comment:
I don't know why you need to delete and new the weak_ptr, can you directly
reset the weak_ptr like `*ls.rpcz_parent_span = span`?
##########
src/brpc/span.cpp:
##########
@@ -37,9 +37,97 @@
#define BRPC_SPAN_INFO_SEP "\1"
-
namespace brpc {
+// Callback for creating a new bthread span when creating a new bthread.
+// This is called by bthread layer when BTHREAD_INHERIT_SPAN flag is set.
+// Returns a heap-allocated weak_ptr<Span>* as void*, or NULL if span creation
fails.
+void* CreateBthreadSpanAsVoid() {
+ const int64_t received_us = butil::cpuwide_time_us();
+ const int64_t base_realtime = butil::gettimeofday_us() - received_us;
+ std::shared_ptr<Span> span = Span::CreateBthreadSpan("Bthread",
base_realtime);
+
+ if (!span) {
+ return NULL;
+ }
+ return new std::weak_ptr<Span>(span);
+}
+
+void DestroyRpczParentSpan(void* ptr) {
+ if (ptr) {
+ delete static_cast<std::weak_ptr<Span>*>(ptr);
+ }
+}
+
+void EndBthreadSpan() {
+ std::shared_ptr<Span> span = GetTlsParentSpan();
+ if (span) {
+ bthread_id_t id = {bthread_self()};
+ span->set_ending_cid(id);
+ }
+
+ ClearTlsParentSpan();
+}
+
+void SetTlsParentSpan(std::shared_ptr<Span> span) {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ }
+ ls.rpcz_parent_span = new std::weak_ptr<Span>(span);
+ BAIDU_SET_VOLATILE_THREAD_LOCAL(tls_bls, ls);
+}
+
+std::shared_ptr<Span> GetTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (!ls.rpcz_parent_span) {
+ return nullptr;
+ }
+
+ auto* weak_ptr = static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
+ return weak_ptr->lock();
+}
+
+void ClearTlsParentSpan() {
+ using namespace bthread;
+ LocalStorage ls = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_bls);
+ if (ls.rpcz_parent_span) {
+ delete static_cast<std::weak_ptr<Span>*>(ls.rpcz_parent_span);
Review Comment:
I think you don't need to delete here, just `ls.rpcz_parent_span->reset()`
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
