feat: structured logging support for zerolog#1031
feat: structured logging support for zerolog#1031aldy505 wants to merge 1 commit intogetsentry:masterfrom
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1031 +/- ##
==========================================
+ Coverage 85.45% 85.65% +0.19%
==========================================
Files 55 56 +1
Lines 5500 5575 +75
==========================================
+ Hits 4700 4775 +75
Misses 655 655
Partials 145 145 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| name: "String array attribute", | ||
| logFunc: func(c *zerolog.Event) *zerolog.Event { | ||
| return c.Strs("foo", []string{"bar", "baz"}) | ||
| }, | ||
| wantAttributes: map[string]sentry.Attribute{ | ||
| "foo.0": {Value: "bar", Type: "string"}, | ||
| "foo.1": {Value: "baz", Type: "string"}, |
There was a problem hiding this comment.
I'm not sure if this is the right behavior. If we see into open-telemetry/opentelemetry-specification#3982, they said if we have an array attribute, it shouldn't be separated like this. But I don't really know the internals of the Trace Explorer.
The options would be:
foo.0=>bar,foo.1=>bazfoo=>["bar", "baz"]
Perhaps @AbhiPrasad can help with this?
There was a problem hiding this comment.
Let's please not unfurl array properties into their own keys. Let's send them as a string for now, with the intention that in the future sentry ingestion will support arrays.
We do this unfurling pattern for message parameters (sentry.message.parameter.0) because we needed to make them indexed + queryable immediately for better product UX in the short term, but I want to avoid arbitrary user arrays to be done this way.
There was a problem hiding this comment.
Good to know. Thanks Abhi!
AbhiPrasad
left a comment
There was a problem hiding this comment.
Instead of using a writer, can we use the hook system in zerolog? https://github.com/rs/zerolog/blob/master/README.md#hooks
I'd like us to avoid the overhead of encoding/decoding json if we can.
something like
type SentryLogHook struct{}
func (h SentryLogHook) Run(e *zerolog.Event, level zerolog.Level, msg string) {
ctx := e.GetCtx()
l := sentry.NewLogger(ctx)
// need more complicated logic to parse the zerolog event
l.WithLevel(convertLevel(level)).Log(ctx, msg)
}
logger := zerolog.New(os.Stdout).Hook(SentryLogHook{})If we do have to use a writer, we should make it async.
| case zerolog.TimestampFieldName: | ||
| continue | ||
| default: | ||
| switch valueType := value.(type) { |
There was a problem hiding this comment.
shouldn't we have a case for int here?
There was a problem hiding this comment.
I did test it with every possible Go types. Yet everything falls down to just 3 types (plus their array variant): string, boolean, float64
There was a problem hiding this comment.
We'll want to still explicitly use attribute.Int because we treat these differently in the backend. In the future there will be operations you can only do on integers you can can't use on floats.
| // Close should not be called directly. | ||
| // It should be called internally by zerolog. | ||
| func (s SentryLogger) Close() error { | ||
| sentry.Flush(time.Second) |
There was a problem hiding this comment.
should we make this configurable?
| l.SetAttributes(attribute.Bool(field, valueType)) | ||
| case float64: | ||
| l.SetAttributes(attribute.Float64(field, float64(valueType))) | ||
| case []any: |
There was a problem hiding this comment.
we should have a default case here that just serializes the value and sends as a string type.
| hub = sentry.NewHub(nil, sentry.NewScope()) | ||
| } | ||
|
|
||
| ctx = sentry.SetHubOnContext(context.Background(), hub) |
There was a problem hiding this comment.
| ctx = sentry.SetHubOnContext(context.Background(), hub) | |
| ctx = sentry.SetHubOnContext(ctx, hub) |
There was a problem hiding this comment.
No can't do. You must assume that the previous ctx is nil.
There was a problem hiding this comment.
really? do you have links to code/docs about this?
| var message string | ||
| for field, value := range evt { | ||
| switch field { | ||
| case zerolog.LevelFieldName: |
There was a problem hiding this comment.
these names can be customized: https://github.com/rs/zerolog/blob/master/README.md#customize-automatic-field-names
we prob need a helper
func (s SentryLogger) getFieldNames() (level, message, timestamp string) {
return zerolog.LevelFieldName,
zerolog.MessageFieldName,
zerolog.TimestampFieldName
}
@AbhiPrasad I've tried the Hook, didn't work. The Hook has this interface signature: Run(e *Event, level Level, message string)In which:
Yes we might want to go to this direction. |
|
Can we please discuss implementation prior opening PRs @aldy505. This is really unproductive else. |
wow thats annoying Let's explore the async writer then. Let's discuss this in the GH issue, and then come back to the PR. We might even want to open up a new PR so we don't get lost in the feedback here. |
|
Hey @aldy505, I was already working on the integration, and since there are enough changes from the current version, it will be easier for me to close the PR and take over myself. |
Closes #1029