Add LogRecord API from Go client #43
Conversation
|
@isaachier - Do you think it would make sense to also be able to log these directly on the span? (I know Go only supports it as part of FinishSpanOptions.) But I'm ok starting with it as part of FinishSpanOptions since we could always add that later. |
include/opentracing/span.h
Outdated
| // and <= the finish_steady_timestamp converted to steady timestamp | ||
| // (or SystemTime::now() if finish_steady_timestamp is default-constructed). | ||
| // Otherwise the behavior of FinishWithOptions() is undefined. | ||
| std::vector<LogRecord> log_records; |
There was a problem hiding this comment.
Could we change this to unspecified behavior?
Also,
// and <= the finish_steady_timestamp converted to steady timestamp
I think you mean converted to system timestamp here.
There was a problem hiding this comment.
Correct, my mistake.
|
I was thinking the same thing but the Go version may do that logic intentionally. For example, if I log a record |
|
my advice is to keep api simple, don't introduce features nobody asked for. |
|
@isaachier -- could you fix the comments for this so that we can merge this in? |
|
Sorry yes will do now. |
include/opentracing/span.h
Outdated
| // be set explicitly). Also, they must be >= the Span's start system timestamp | ||
| // and <= the finish_steady_timestamp converted to system timestamp | ||
| // (or SystemTime::now() if finish_steady_timestamp is default-constructed). | ||
| // Otherwise the behavior of FinishWithOptions() is undefined. |
There was a problem hiding this comment.
// Otherwise the behavior of FinishWithOptions() is undefined.
Can we change this to unspecified or remove that line. Undefined behavior would mean a tracer is free to do anything (abort, segfault, etc). Given that converting between steady and system time is a noisy process, I don't think it would be right for a tracer to abort or corrupt the process if a timestamp is out of bounds.
Fix for #35