Add events in SpanData#194
Conversation
Codecov Report
@@ Coverage Diff @@
## master #194 +/- ##
==========================================
+ Coverage 95.45% 95.51% +0.05%
==========================================
Files 87 91 +4
Lines 2488 2521 +33
==========================================
+ Hits 2375 2408 +33
Misses 113 113
|
pyohannes
left a comment
pyohannes
left a comment
There was a problem hiding this comment.
That looks good, thanks!
Would you be willing to work on extending the Recordable interface by an AddEvent method that supports attributes? And probably also by an AddLink method?
There's lots of work going on in exporter development now, and having those methods sooner rather than later will spare us painful work in the future (as any update to Recordable breaks compatibility for exporters).
|
@pyohannes yes, that sounds good! I can add those in a future PR. |
pyohannes
left a comment
pyohannes
left a comment
There was a problem hiding this comment.
This looks good, thanks.
pyohannes
added
the
pr:please-merge
|
LGTM :) |
IlyaKobelevskiy
left a comment
IlyaKobelevskiy
left a comment
There was a problem hiding this comment.
Few minor nits, but overall looks good!
| std::string name_; | ||
| core::SystemTimestamp timestamp_; |
There was a problem hiding this comment.
Both name_ and timestamp_ can be const membes since they are only set in ctor.
| * Get the name for this event | ||
| * @return the name for this event | ||
| */ | ||
| std::string GetName() const noexcept { return name_; } |
There was a problem hiding this comment.
std::string -> -> opentelemetry::nostd::string_view (for consistency with how SpanData returns string members, and it also should avoid copy).
| { | ||
| public: | ||
| SpanDataEvent(std::string name, core::SystemTimestamp timestamp) | ||
| : name_(name), timestamp_(timestamp) |
There was a problem hiding this comment.
std::string -> opentelemetry::nostd::string_view (for consistency with how SpanData sets string members).
| ASSERT_EQ(data.GetDuration(), std::chrono::nanoseconds(1000000)); | ||
| ASSERT_EQ(opentelemetry::nostd::get<int64_t>(data.GetAttributes().at("attr1")), 314159); | ||
| ASSERT_EQ(data.GetEvents().at(0).GetName(), "event1"); | ||
| ASSERT_EQ(data.GetEvents().at(0).GetTimestamp(), now); |
There was a problem hiding this comment.
I think ASSERT_EQ can be replaced by EXPECT_EQ throughout this file.
…s-create-or-update-comment-digest Update peter-evans/create-or-update-comment digest to 5a1e4b7
5 participants
Implement
AddEventfor SpanData. Currently onlynameandtimestampcan be set due to limitations in therecordableinterface. In the future, attributes should also be supported in events.