Volfdb#285
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #285 +/- ##
===========================================
+ Coverage 71.15% 71.18% +0.03%
===========================================
Files 370 370
Lines 23457 23673 +216
Branches 2463 2492 +29
===========================================
+ Hits 16690 16851 +161
- Misses 6767 6822 +55 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
9362fff to
1bb317b
Compare
| archive(key, msg.data(), msg.length(), tracingID); | ||
| } | ||
| void FDB::archive(eckit::DataHandle& handle) { | ||
| void FDB::archive(eckit::DataHandle& handle, const std::string& tracingID) { |
There was a problem hiding this comment.
A passing comment: I would use class TracingContext or something similar rather than just a std::string. It seems plausible that this should be something that we might want to be extendable / structured.
e4d1c46 to
e65b90d
Compare
| // std::lock_guard<std::mutex> lock(initMutex); | ||
| // if (!instance_) { | ||
| // instance_.reset(new ClientConnectionRouter()); | ||
| // } | ||
| // return *instance_; |
| MoveVisitor::MoveVisitor(eckit::Queue<MoveElement>& queue, const metkit::mars::MarsRequest& request, | ||
| const eckit::URI& dest) : | ||
| QueryVisitor<MoveElement>(queue, request), dest_(dest) {} | ||
| const std::string& tracingID, const eckit::URI& dest) : |
There was a problem hiding this comment.
Regarding tracing, I really think it would be better to use a thread_local static variable or similar rather than updating many many functions and passing a const string around everywhere.
| internal_->registerFlushCallback(callback); | ||
| } | ||
|
|
||
| std::string FDB::generateTracingID(const std::string& prefix) { |
There was a problem hiding this comment.
I think we should format this as a W3C traceparent (00-<trace-id>-<parent-id>-<flags>), i'd keep it as a string we pass around, but format it inline with the standard https://www.w3.org/TR/trace-context/.
- it lines up with our codex (https://github.com/ecmwf/codex/blob/main/Guidelines/Observability.md#4-logging-standard) - traceId/spanId in logs for correlation, traceparent gives us those.
- we could optionally let the public fdb interface accept a traceparent input. Then any wrapping services or apps that already has a trace can pass theirs and our work hangs off it instead of floating as its own root.
- traceparent gives us depth context: trace-id stays constant for the whole request, parent-id is the caller's span-id, so hops can point back at their parent and you get a tree rather than a collection of logs with same id.
At the moment generateTracingID("archive") bakes "archive" into the id, but I'd argue that should be exposed as an attribute on a span context, not part of the identity. I know we're just talkign about Id structure at the moment but with a proper traceparent it would allow us to eventually capture fdb interactions something like this:
trace_id = 4bf92f3577b34da6a3ce929d0e0e4736
|
+- span "fdb.archive" span_id=00f067aa0ba902b7 parent= (root)
+- span "store.archive" span_id=b7ad6b7169203331 parent=00f067aa0ba902b7 (data blob)
+- span "catalogue.archive" span_id=9d6f2c1b4e7a8053 parent=00f067aa0ba902b7 (index entry)
5 participants
Description
Contributor Declaration
By opening this pull request, I affirm the following:
🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/fdb/pull-requests/PR-285