ext: Add LogFields helpers for keys from specification #226
ext: Add LogFields helpers for keys from specification #226
Conversation
e014616
to
222e191
Compare
Codecov Report
@@ Coverage Diff @@
## master #226 +/- ##
=========================================
Coverage ? 60.07%
=========================================
Files ? 15
Lines ? 844
Branches ? 0
=========================================
Hits ? 507
Misses ? 297
Partials ? 40
Continue to review full report at Codecov.
|
ext/fields.go
Outdated
var ( | ||
LogEvent = stringLogName("event") | ||
LogMessage = stringLogName("message") | ||
LogStack = stringLogName("stack") |
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.
I think these would make more sense in the log
pkg
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.
also, only the first of these is self-contained Field, the others don't make sense as fields, they are just keys. They would make more sense as functions taking the value string and returning a Field.
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.
I think these would make more sense in the
log
pkg
Sound reasonable, but LogError() depends on opentrace.Span so this result in cyclic dependency between opentrace and log packages.
ext/fields.go
Outdated
span.LogFields(log.String(string(logName), value)) | ||
} | ||
|
||
type stringLogName string |
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.
declaration should precede the functions
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.
Ok, fixed in new version
222e191
to
c747d25
Compare
log/field.go
Outdated
// Stack adds a string-valued field with name "stack" to a Span.LogFields() record | ||
func Stack(val string) Field { | ||
return String("stack", val) | ||
} |
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.
why is "stack" a string? Is the intention to represent a stack trace?
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.
It was declared that way in semantic conventions doc
| "stack" | string | A stack trace in platform-conventional format; may or may not pertain to an error. E.g., "File \"example.py\", line 7, in \<module\>\ncaller()\nFile \"example.py\", line 5, in caller\ncallee()\nFile \"example.py\", line 2, in callee\nraise Exception(\"Yikes\")\n"
I tend to agree that this may be not suitable for some implementations, so I can drop that function.
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.
I think the convention is a bit misleading here. Once the tag is set and accepted by the span (e.g. when the trace is sampled), then yes, it has to be a string, as there's no other data structure available (NB: OpenTelemetry will have a proper structure for stack frames). But that doesn't mean that the tag must be set as a string, because doing so would be quite inefficient if the trace is not sampled. I always prefer the API to take native objects (e.g. https://golang.org/pkg/runtime/debug/#Stack, or a Throwable in Java) and the tracer implementation can decide to convert them to strings.
So yes, I recommend removing this, as it's not required by the rest of the PR.
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.
Agree. Fixed in new version.
c747d25
to
b8d8b85
Compare
log/field.go
Outdated
@@ -143,6 +143,16 @@ func Object(key string, obj interface{}) Field { | |||
} | |||
} | |||
|
|||
// Event adds a string-valued field with name "value" to a Span.LogFields() record |
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.
description is not accurate. Suggest:
Event creates a string-valued Field for span logs with key="event" and value=val.
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.
Ok, fixed in fresh version. Thank you for your patience.
log/field.go
Outdated
return String("event", val) | ||
} | ||
|
||
// Message adds a string-valued field with name "event" to a Span.LogFields() record |
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.
Message creates a string-valued Field for span logs with key="message" and value=val.
ext/field.go
Outdated
"github.com/opentracing/opentracing-go/log" | ||
) | ||
|
||
// LogError add error event record for the Span |
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.
LogError sets the error=true tag on the Span and logs err as an "error" event.
Opentracing spec specify prefered names for log messages [1] This patch adds declaration for fields which are meaningful for golang - log: add helpers function for spec keys - ext: add LogError() helper for error LogError() helper can not be declarated in log package because it depends opentrace.Span which result in cyclic depencency Footnotes: [1] https://github.com/opentracing/specification/blob/master/semantic_conventions.md#log-fields-table
b8d8b85
to
63fe2a1
Compare
Thanks for the PR. |
Opentracing spec specify prefered names for log messages [1]
This patch adds declaration for fields which are meaningful for golan
I've added only minimal subset of features as a first step,
For example this patch has no stacktrace logging for errors because stacktrace generation is implementation depended, so caller should do it explicitly.
Footnotes:
[1] https://github.com/opentracing/specification/blob/master/semantic_conventions.md