ext: Add LogFields helpers for keys from specification #226
Conversation
dmonakhov
force-pushed
the
feature/ext-log_fields_helpers
branch
from
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.
I think these would make more sense in the log pkg
There was a problem hiding this comment.
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.
I think these would make more sense in the
logpkg
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.
declaration should precede the functions
There was a problem hiding this comment.
Ok, fixed in new version
dmonakhov
force-pushed
the
feature/ext-log_fields_helpers
branch
from
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.
why is "stack" a string? Is the intention to represent a stack trace?
There was a problem hiding this comment.
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.
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.
Agree. Fixed in new version.
dmonakhov
force-pushed
the
feature/ext-log_fields_helpers
branch
from
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.
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.
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.
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.
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
dmonakhov
force-pushed
the
feature/ext-log_fields_helpers
branch
from
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