-
Notifications
You must be signed in to change notification settings - Fork 344
Apply the builder pattern to the Span.log(..) methods. #14
base: interfaces
Are you sure you want to change the base?
Conversation
EventBuilder withPayload(Object payload); | ||
|
||
/** | ||
* Add a specific timestamp, in microseconds in UTC time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in microseconds since Unit epoch (I don't think UTC is relevant for epoch-based ts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quite right.
097c9e8
to
14ac604
Compare
**/ | ||
Span log(long instantMicroseconds, String eventName, /* @Nullable */ Object payload); | ||
EventBuilder buildEvent(String eventName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requiring an eventName
makes this a bad fit for info/error debug logging... we could still have buildEvent()
as a sibling to info/error, though I imagine you're not a fan of that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the event name synonymous with event message (a la log message)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per http://opentracing.io/spec,
The event name should be the stable identifier for some notable moment in the lifetime of a Span. For instance, a Span representing a browser page load might add an event for each of the Performance.timing moments. While it is not a formal requirement, specific event names should apply to many Span instances: tracing systems can use these event names (and timestamps) to analyze Spans in the aggregate.
(These are the Zipkin semantics, roughly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name taken out of constructor and added via withName(string)
3056c7b
to
cae53ac
Compare
14ac604
to
79383c7
Compare
cae53ac
to
7bc6ae1
Compare
I'm ambivalent about this PR... I think it's "clean" in the sense of being extensible and clear, but inconvenient (in the sense that simple logging statements require three chained method calls). Since Java has abstract classes, I suppose we could take things in that direction, too... the builder interface would be there for plumbing but
(etc) My inclination would be to keep Span as a pure interface and just add separate methods for the separate sorts of logging that need to happen, as I tend to think of them as being mutually exclusive. But, again, I'm ambivalent about this, and Java's idioms often involve lots of boilerplate (for better or worse) so I am uneasy arguing against this approach for that reason. |
i'm happy to leave this PR open into the future just as a reference point. That said, I do like the idea of the logging api going through an EventBuilder as its the more natural display of the concepts of tracing with layers added on. |
🆒, sounds good. Thanks @michaelsembwever |
Similar to what was done to Tracer.createSpan(..) --> Tracer.buildSpan(..)
Again I don't know how far we can interpret the Specification.
But taking this approach does make it cleaner to add logging capabilities afterwards, imo.