[EXAMPLE] Add manual asynchronous context propagation example#3935
[EXAMPLE] Add manual asynchronous context propagation example#3935jgomezselles wants to merge 8 commits intoopen-telemetry:mainfrom
Conversation
This PR adds an example that demonstrates tracing async requests between generic clients and servers. It simulates context injections and extractions and shows how to explicitly set parent span_id's in different situations, when the scope is not available, or different threads are run in parallel. The proposed pattern may be used for any kind of asynchronous client/server communication. It defines auxiliary functions that can be easily adapted to other code bases together with a README documenting the flow and expected outcome. Apart from markdown lining and clang formatting, BUILD and relevant CMakeLists have been adapted.
|
Hi! First I wanted to say that I'm a big fan of this project. I've been involved with OpenTelemetry and C++ in different ways in the past, When I started trying to include this lib some years ago, I have to confess struggled a bit. Since then, I really wanted to contribute with an example like this, so folks can just easily create spans in regular C++ code bases that deal with async calls. Finally I had some quality time. Of course, I'm very open for feedback. I tried my best to clearly explain this with a simple example that doesn't need to rely on other libs or clients and servers. I hope this makes sense. For context: I'm using this kind of implementation in an async http2 client I created for load testing and demos (repo here ). If this is eventually approved, I'd like to also contribute to the documentation. Thanks again, and congrats on the great work maintaining this library! |
|
Thanks for the PR. Non-blocking: this example is correct, but it reads more like a carrier-based context propagation example than an async programming example. Since we already have an HTTP propagation example, this would be more distinctive if it either (a) were renamed/reframed around manual async context propagation, or (b) showed a more practical in-process async pattern such as passing SpanContext from the submitting thread to worker-thread child spans. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3935 +/- ##
==========================================
+ Coverage 90.06% 90.13% +0.07%
==========================================
Files 226 226
Lines 7227 7227
==========================================
+ Hits 6508 6513 +5
+ Misses 719 714 -5 🚀 New features to boost your workflow:
|
|
Thanks for the feedback. I'll think about it and will come back with changes. |
After the changes, the new reframing should be: This PR adds an example that demonstrates how to manually propagate tracing context across asynchronous calls between generic clients and servers. It simulates context injections and extractions showing how to **explicitly/manually** set parent span_id's in different situations, when the scope is not available, or different threads are run in parallel. The proposed pattern may be helpful for use cases which involve asynchronous operations. It defines auxiliary functions that can be easily adapted to other code bases together with a README documenting the flow and expected outcome. Apart from markdown lining and clang formatting, BUILD and relevant CMakeLists have been adapted.
|
Hi again, Thanks again for the feedback. I've been thinking about which of the proposed options to take. I finally moved on with the first one, because I want to show something more than just an async example. It is absolutely true that it went far from just that simple async example, so I'm reframing it. Now, I think it's more clear what I'm trying to show: how to manually propagate context in a couple of cases. They include the asynchronous case, but that's a detail. Changes added:
I hope this is better now. Thanks again! |
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've shared some comments and a question below.
examples/manual_propagation/main.cc
Outdated
| namespace nostd = opentelemetry::nostd; | ||
| namespace ctx = opentelemetry::context; | ||
|
|
||
| using header_map = std::map<std::string, std::string>; |
There was a problem hiding this comment.
Please use the Google naming conventions in this file for the types and functions.
There was a problem hiding this comment.
Sorry I missed it. I'm applying changes in the next commit.
BTW, I left the get_tracer as in the other examples to keep the same notation. Please, let me know if it would be useful to make another PR changing all occurrences, and I'll work on that.
| } | ||
| } // namespace | ||
|
|
||
| namespace utils |
There was a problem hiding this comment.
Please fix the clang-tidy warnings from this file:
https://github.com/open-telemetry/opentelemetry-cpp/actions/runs/23163761181
opentelemetry-cpp/examples/manual_propagation/main.cc (7 warnings)
| Line | Check | Message |
|---|---|---|
| 74 | misc-use-internal-linkage |
function 'create_span' can be made static or moved into an anonymous namespace to enforce internal linkage |
| 83 | misc-use-internal-linkage |
function 'create_child_span' can be made static or moved into an anonymous namespace to enforce internal linkage |
| 119 | cppcoreguidelines-avoid-const-or-ref-data-members |
member 'headers_' of type 'header_map &' (aka 'map<basic_string, basic_string> &') is a reference |
| 122 | misc-use-internal-linkage |
function 'inject_trace_context' can be made static or moved into an anonymous namespace to enforce internal linkage |
| 138 | misc-use-internal-linkage |
function 'create_child_span_from_remote' can be made static or moved into an anonymous namespace to enforce internal linkage |
| 156 | misc-use-internal-linkage |
function 'process_answer' can be made static or moved into an anonymous namespace to enforce internal linkage |
| 171 | misc-use-internal-linkage |
function 'process_request' can be made static or moved into an anonymous namespace to enforce internal linkage |
There was a problem hiding this comment.
Ack thanks for pointing that output out.
- Applying the anonymous namespace for all of them in the next commit.
- Regarding the header ref, I'll use
std::reference_wrapper(same pattern than in other examples, and we actually need it to be a ref).
examples/manual_propagation/main.cc
Outdated
| return span; | ||
| } | ||
|
|
||
| nostd::shared_ptr<trace_api::Span> create_child_span( |
There was a problem hiding this comment.
not blocking - Consider consolidating the create_span and create_child_span functions into one
There was a problem hiding this comment.
Fair enough, thanks. My main blocker was not being able to figure out how to default to a nullptr within the nostd, and I dodn't want to create a shared_ptr of a span every call.
But, I'll default it to const nostd::shared_ptr<trace_api::Span> &parent = nostd::shared_ptr<trace_api::Span>{} , which actually would make it IIUC.
examples/manual_propagation/main.cc
Outdated
|
|
||
| void Set(nostd::string_view k, nostd::string_view v) noexcept override | ||
| { | ||
| headers_.emplace(std::string(k.data(), k.size()), std::string(v.data(), v.size())); |
There was a problem hiding this comment.
Should this line overwrite the value if the key already exists?
There was a problem hiding this comment.
It should! Thanks. I would use insert_or_apply, but I think it's not available for cpp14. I'll use brackets instead.
| function is needed for the injection to explicitly use the desired span | ||
| context. | ||
|
|
||
| ```cpp |
There was a problem hiding this comment.
not blocking - consider moving these notes to comments in the example main.cc and removing code blocks here.
There was a problem hiding this comment.
Done, sorry for the verbosity. As you point in the next comment, the maintainability burden would not be great. Moved and simplified.
| } | ||
| ``` | ||
|
|
||
| The previous examples make use of the following aliases: |
There was a problem hiding this comment.
not blocking - consider removing this note on aliases. Duplicate code in the README and main.cc may not add much value while increasing maintenance overhead.
This commit removes the code duplicated in the README for maintainability. It also changes * for - as markdownlint was complainign.
- Using Google naming conventions (but in get_tracer) - clang-tidy: Anon ns for utils funtions and map ref - Consolidating CreateSpan functions - Overwrite values when setting carrier
|
Thanks for the review! I did my best to apply the changes. Please let me know if there are more things to improve (I'll keep an eye on the linters, etc too) |
| // Simulating work with nested spans | ||
| server_span->AddEvent("Processing in server"); | ||
| auto nested = utils::CreateSpan("nested", trace_api::SpanKind::kServer, server_span); | ||
| nested->AddEvent("Nested did some work"); |
There was a problem hiding this comment.
Let's not use the Span Event API here. It's planned to be deprecated - https://github.com/open-telemetry/opentelemetry-specification/blob/fe98cb6e7d579b1360eb856670ac08a2133131d9/oteps/4430-span-event-api-deprecation-plan.md
|
This is a nice, clean example, but I’m not sure it adds much on top of the existing HTTP client/server propagation example, which already covers the real-world use case. Also, using More broadly, I’m not sure we need a separate “generic carrier” example here. In practice, users almost always interact with propagation through concrete transports like HTTP or gRPC, and the HTTP example already demonstrates the pattern clearly. As it stands, this feels a bit overlapping and may unintentionally suggest a pattern we don’t actually recommend. |
Fixes #1464
Changes
This PR adds an example that demonstrates tracing async requests between generic clients and servers. It simulates context injections and extractions and shows how to explicitly set parent span_id's in different situations, when the scope is not available, or different threads are run in parallel.
The proposed pattern may be used for any kind of asynchronous client/server communication.
It defines auxiliary functions that can be easily adapted to other code bases together with a README documenting the flow and expected outcome.
Apart from markdown lining and clang formatting, BUILD and relevant CMakeLists have been adapted.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes