impl(otel): set Recordable identity and timestamps #11286
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #11286 +/- ##
=======================================
Coverage 93.77% 93.77%
=======================================
Files 1767 1767
Lines 159222 159222
=======================================
+ Hits 149316 149317 +1
+ Misses 9906 9905 -1 see 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| opentelemetry::trace::SpanContext const& /*span_context*/, | ||
| opentelemetry::trace::SpanId /*parent_span_id*/) noexcept {} | ||
| opentelemetry::trace::SpanContext const& span_context, | ||
| opentelemetry::trace::SpanId parent_span_id) noexcept { |
There was a problem hiding this comment.
I'm not sure how you can assert noexcept when you do things like construct strings.
There was a problem hiding this comment.
You can't. These are virtual functions and we need to respect the specification.
I tried (and failed) to convince these folks to drop the noexcept.
It seems that we will need to capture all exceptions. Maybe something like this (
// in some header...
bool NoExceptAction(absl::FunctionRef<void()> action) noexcept;
// in some cc
#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
bool NoExceptAction(absl::FunctionRef<void()> action) noexcept try {
action();
return true;
} catch(...) {
return false;
}
#else
bool NoExceptAction(absl::FunctionRef<void()> action) noexcept {
action(); // Will crash, would have crashed anyway.
return true;
}
#endif
// In Recordable.cc
void Recordable::SetIdentity(...) {
usable_ = usable_ & NoExceptAction([&]() { SetIdentityImpl(...); });
}Then we will drop (not send to Cloud Trace) any Recordables where usable_ is false.
This is all for a TODO bug and future PRs.
| std::array<char, 2 * opentelemetry::trace::TraceId::kSize> hex_trace_buf; | ||
| span_context.trace_id().ToLowerBase16(hex_trace_buf); | ||
| std::string const hex_trace(hex_trace_buf.data(), | ||
| 2 * opentelemetry::trace::TraceId::kSize); |
There was a problem hiding this comment.
hex_trace_buf.size() perhaps, to avoid repeating the template parameter, and to perpetuate the "data/size" pairing convention.
dbolduc
force-pushed
the
otel-recordable-identity-and-timestamps
branch
from
c6e046a to
26875f5
Compare
| // std::chrono::system_clock may not have nanosecond resolution on some | ||
| // platforms, so we avoid using it for conversions between OpenTelemetry | ||
| // time and Protobuf time. |
There was a problem hiding this comment.
Looks good. It seems like opentelemetry::common::SystemTimestamp's implicit conversion to std::chrono::system_clock::time_point is a rounding accident waiting to happen.
There was a problem hiding this comment.
Yeah, thanks for the warning.
| // platforms, so we avoid using it for conversions between OpenTelemetry | ||
| // time and Protobuf time. | ||
| auto t = absl::FromUnixNanos(start_time.time_since_epoch().count()); | ||
| *span_.mutable_start_time() = internal::ToProtoTimestamp(std::move(t)); |
There was a problem hiding this comment.
absl::Time is one of those always-pass-by-value exceptions, so drop the move().
Part of the work for #11156
Implement the identity and timestamp setters for a
Recordable. This change should make the class usable with Cloud Trace.@devbww might be interested in the
absl::Timebusiness.This change is