gemmellr commented on a change in pull request #319:
URL: https://github.com/apache/qpid-proton/pull/319#discussion_r692955572



##########
File path: cpp/src/tracing.cpp
##########
@@ -168,32 +168,25 @@ void on_message_start_span(message &message, delivery &d) 
{
 
     trace::Scope scope = get_tracer()->WithActiveSpan(span);
 
-    if (delivery_state == "unknown")
-    {
-        span->AddEvent("Delivery state is unknown");
-    }
-    if (delivery_state == "Received")
-    {
-        span->AddEvent("Delivery is received");
-    }
-
     tag_span[tag_in_binary] = span;
 }
 
-void on_message_end_span(delivery &d) {
+void on_message_end_span(delivery &d, tracker &track) {
     binary tag = d.tag();
     nostd::shared_ptr<trace::Span> span = tag_span[tag];
-    // std::cout << "Inside on_message_end_span, before end" << std::endl;
+    transfer tt(track);
+    std::string delivery_state = to_string(tt.state());
+    span->AddEvent("delivery state: "+ delivery_state);
     span->End();
-    // std::cout << "Inside on_message_end_span, after end" << std::endl;
 }
 
-void on_settled_span(delivery &d) {
+void on_settled_span(delivery &d, tracker &track) {
     binary tag = d.tag();
     nostd::shared_ptr<trace::Span> span = tag_span[tag];
-    // std::cout << "Inside on_settled_span, before end" << std::endl;
+    transfer tt(track);
+    std::string delivery_state = to_string(tt.state());
+    span->AddEvent("delivery state: "+ delivery_state);

Review comment:
       Similarly, I might omit this one if not set. Though its less clear given 
this point does mark the end of the message handling from the senders 
perspective as the other peer is done with the message settled (whereas with 
the on_message_end_span ending, that completing just means the handler 
returned, it doesnt necessarily mean the processing is done).
   
   @ssorj thoughts?

##########
File path: cpp/src/tracing.cpp
##########
@@ -168,32 +168,25 @@ void on_message_start_span(message &message, delivery &d) 
{
 
     trace::Scope scope = get_tracer()->WithActiveSpan(span);
 
-    if (delivery_state == "unknown")
-    {
-        span->AddEvent("Delivery state is unknown");
-    }
-    if (delivery_state == "Received")
-    {
-        span->AddEvent("Delivery is received");
-    }
-
     tag_span[tag_in_binary] = span;
 }
 
-void on_message_end_span(delivery &d) {
+void on_message_end_span(delivery &d, tracker &track) {
     binary tag = d.tag();
     nostd::shared_ptr<trace::Span> span = tag_span[tag];
-    // std::cout << "Inside on_message_end_span, before end" << std::endl;
+    transfer tt(track);
+    std::string delivery_state = to_string(tt.state());
+    span->AddEvent("delivery state: "+ delivery_state);

Review comment:
       I would possibly omit doing this if there isnt any delivery state yet 
(which is a possibility, should the application or the client not have finished 
processing the message and set one yet)

##########
File path: .github/workflows/build.yml
##########
@@ -60,9 +66,59 @@ jobs:
       if: runner.os == 'macOS'
       run: |
         brew install libuv swig pkgconfig jsoncpp
+    - name: Checkout curl repo (for jaeger)

Review comment:
       As mentioned on IRC, I expect there is a package that can be installed 
to provide whatever e.g development headers are needed by the other build 
(until such time later that it too can just be used via packages).
   
   For now this works so its good to concentrate on the other actual tracing 
aspects first and come back to it later.




-- 
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]

Reply via email to