-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Structured Tags #22
Conversation
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.
lgtm, just a nit
context.go
Outdated
|
||
// STag is a structured tag. | ||
type STag struct { | ||
// A unique key for the structure that your logging |
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.
seems like this sentence might be incomplete
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.
Apparently I've forgotten third grade grammar :-(
e4f0d46
to
92ec5be
Compare
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.
a few comments
context.go
Outdated
type structuredKey struct{} | ||
|
||
var stringCtxKey = &stringKey{} | ||
var structuredCtxKey = &structuredKey{} |
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.
Hmm, let's make these two key-types non-pointers. It's weird and adds state in places none is necessary.
context.go
Outdated
new := make([]STag, len(old)+len(tags)) | ||
copy(new, old) | ||
for i := range new[len(old):] { | ||
new[len(old)+i] = tags[i] | ||
} |
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.
Hmm, this is a very awkward way to spell new = append(old[:len(old):len(old)], tags...)
(if this were go 1.21+ we could use new = append(slices.Clip(old), tags...)
or even better new = slices.Concat(old, tags)
-- https://pkg.go.dev/slices#Clip)
Also, new
is a keyword, I try to avoid using it as a symbol-name.
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.
Hmm -- I guess I didn't really think of it that way. Your way is definitely more concise, so I went with that.
emitter/textlog/emitter.go
Outdated
} | ||
m.WriteString(p.Key) | ||
m.WriteByte('=') | ||
m.WriteString(fmt.Sprintf("%+v", p.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.
It would probably be better to use fmt.Fprintf(m, "%+v", p.Val)
here.
(it will often boil down to something equivalent, but it can also avoid some extra allocations)
emitter/jsonlog/emitter.go
Outdated
} else { | ||
jsonString(b, "json marshal err: "+marshalErr.Error()) | ||
} | ||
b.WriteString("}, ") |
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.
Hmm, I feel like this b.WriteString()
should be outside the loop.
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.
There were actually a couple bugs here, but I fixed them an updated the test.
92ec5be
to
32e3256
Compare
Adds `STags` to `Entry` and delegates marshalling those structures to the emitters in whatever format is most appropriate. Structured tags are added via the alog.AddStructuredTags(ctx, sTag1, sTag2...) function.
32e3256
to
6ccbc51
Compare
Adds
STags
toEntry
and delegates marshalling those structures to the emitters in whatever format is most appropriate.Structured tags are added via the alog.AddStructuredTags(ctx, sTag1, sTag2...) function.