Logging Bridge For ZeroLog #5761
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Hello @MrAlias, i have made the corrections. Is there anything you would like me to add |
CHANGELOG.md
Outdated
There was a problem hiding this comment.
A changelog entry is required.
| @@ -0,0 +1,23 @@ | |||
| module go.opentelemetry.io/contrib/bridges/otelzerolog | |||
|
|
|||
| go 1.22.1 | |||
There was a problem hiding this comment.
This repo currently needs to support Go 1.21.
There was a problem hiding this comment.
I resolved this in #5782
|
|
||
| // Levels returns the list of log levels we want to be sent to OpenTelemetry. | ||
| func (h *SeverityHook) Levels() zerolog.Level { | ||
| return h.levels |
There was a problem hiding this comment.
This is never set, it will always be empty.
There was a problem hiding this comment.
I just set it in the Severity Struct
bridges/otelzerolog/hook.go
Outdated
| record.SetTimestamp(zerolog.TimestampFunc()) | ||
| record.SetBody(log.StringValue(msg)) | ||
| const sevOffset = zerolog.Level(log.SeverityDebug) - zerolog.DebugLevel | ||
| record.SetSeverity(log.Severity(level + sevOffset)) |
There was a problem hiding this comment.
This assumes the same scaling factor. OTel levels to not scale at this rate: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-severitynumber
bridges/otelzerolog/hook.go
Outdated
| func extractFields(_ *zerolog.Event) map[string]interface{} { | ||
| // Here you would implement the logic to extract fields from the zerolog event | ||
| // This might involve using reflection or zerolog internals if necessary | ||
| fields := make(map[string]interface{}) |
There was a problem hiding this comment.
This is ... worrying, it is allocating a map just to make a conversion. Can the zero log fields not be directly converted sent to SetAttribute and skip this allocation?
bridges/otelzerolog/hook.go
Outdated
| } | ||
|
|
||
| func convertAttribute(key string, value interface{}) log.KeyValue { | ||
| switch v := value.(type) { |
bridges/otelzerolog/hook.go
Outdated
| } | ||
|
|
||
| func convertFields(fields map[string]interface{}, msg string) []log.KeyValue { | ||
| kvs := make([]log.KeyValue, 0, len(fields)) |
There was a problem hiding this comment.
Similar here, this is making an allocation that is going to be potentially thrown away every time a message is emitted.
bridges/otelzerolog/hook_test.go
Outdated
There was a problem hiding this comment.
There is a fair amount of test coverage missing. If we plan to implement a method or function in the PR it should be accompanied with comprehensive tests.
If you need to break this into smaller PRs, adding a method/function stub that you will implement and test in a follow-up PR is acceptable. You will need to track that as an issue though and keep the bridge unreleased until all parts are complete.
There was a problem hiding this comment.
Thank you @MrAlias
I will do just that
There was a problem hiding this comment.
I wanted to check in regarding this PR. While I haven't finished all the implementations yet, are the functions I've initialized sufficient, or do I need to add any additional functions? @MrAlias
There was a problem hiding this comment.
I am more asking about adding test coverage for the functions that are included in this PR. There is a considerable amount of functionality being introduced here that is not tested.
There was a problem hiding this comment.
Understood
Thank you
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5761 +/- ##
=======================================
- Coverage 64.6% 64.3% -0.3%
=======================================
Files 202 201 -1
Lines 12591 12615 +24
=======================================
- Hits 8134 8118 -16
- Misses 4221 4260 +39
- Partials 236 237 +1
|
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
part of #5405