Fix panic for nil attributes and move convert funcs to internal/shared/logutil #6237

m1heng · 2024-10-12T08:15:50Z

This pull request aligns the convertValue function in the otellogrus package with the otellogr.
Also fix #6235

pellared · 2024-10-12T10:07:03Z

It would be better to put convertValue code under internal/shared and use //go:generate gotmpl to reuse the code in log bridges (otellogrus, otelzap, otellogr). Do you want to try doing it?

m1heng · 2024-10-13T03:10:59Z

@pellared just did it, also added some nil pointer test.
Also this would leads to a semi-breaking change.
e.g.

in otellogrus, time attr would become nanoseconds and many other unhandled attribute type.
in otellogrus and otelzap unsupported type is marked as unhandled attribute type: (%s) %+v, but now would be unhandled: (%s) %+v.

pellared

Also this would leads to a semi-breaking change.

I think these changes are good 👍

internal/shared/logutil/convert.go.tmpl

codecov · 2024-10-13T21:58:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.9%. Comparing base (4e909f0) to head (3f5847a).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #6237     +/-   ##
=======================================
+ Coverage   66.5%   66.9%   +0.4%     
=======================================
  Files        188     190      +2     
  Lines      12456   12538     +82     
=======================================
+ Hits        8284    8391    +107     
+ Misses      3879    3856     -23     
+ Partials     293     291      -2

Files with missing lines	Coverage Δ
bridges/otellogr/convert.go	`100.0% <ø> (+0.9%)`	⬆️
bridges/otellogr/logsink.go	`98.6% <100.0%> (+0.4%)`	⬆️
bridges/otellogrus/convert.go	`100.0% <100.0%> (ø)`
bridges/otellogrus/hook.go	`97.5% <ø> (-0.1%)`	⬇️
bridges/otelzap/convert.go	`100.0% <100.0%> (ø)`
bridges/otelzap/encoder.go	`100.0% <ø> (+13.8%)`	⬆️

... and 1 file with indirect coverage changes

pellared · 2024-10-13T21:59:17Z

Can you also please update the changelog and add an entry for each package where #6235 is fixed.

m1heng · 2024-10-14T14:02:35Z

done

internal/shared/logutil/convert.go.tmpl

Co-authored-by: Robert Pająk <pellared@hotmail.com>

m1heng · 2024-10-14T16:32:53Z

you want func ConvertValue to be private in all package?

pellared · 2024-10-14T16:33:54Z

you want func ConvertValue to be private in all package?

Correct. We do not want to extend the exported API surface unless necessary.

Co-authored-by: Robert Pająk <pellared@hotmail.com>

m1heng · 2024-10-14T16:51:05Z

just changed to private

pellared · 2024-10-15T08:56:02Z

@m1heng, thank you very much for your contribution 🏅

Reuse convertValue in otelslog. Follows #6237.

### Added - Add the `WithSource` option to the `go.opentelemetry.io/contrib/bridges/otelslog` log bridge to set the `code.*` attributes in the log record that includes the source location where the record was emitted. (#6253) - Add `ContextWithStartTime` and `StartTimeFromContext` to `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp`, which allows setting the start time using go context. (#6137) - Set the `code.*` attributes in `go.opentelemetry.io/contrib/bridges/otelzap` if the `zap.Logger` was created with the `AddCaller` or `AddStacktrace` option. (#6268) - Add a `LogProcessor` to `go.opentelemetry.io/contrib/processors/baggagecopy` to copy baggage members to log records. (#6277) - Use `baggagecopy.NewLogProcessor` when configuring a Log Provider. - `NewLogProcessor` accepts a `Filter` function type that selects which baggage members are added to the log record. ### Changed - Transform raw (`slog.KindAny`) attribute values to matching `log.Value` types. For example, `[]string{"foo", "bar"}` attribute value is now transformed to `log.SliceValue(log.StringValue("foo"), log.StringValue("bar"))` instead of `log.String("[foo bar"])`. (#6254) - Upgrade `go.opentelemetry.io/otel/semconv/v1.17.0` to `go.opentelemetry.io/otel/semconv/v1.21.0` in `go.opentelemetry.io/contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo`. (#6272) - Resource doesn't merge with defaults if a valid resource is configured in `go.opentelemetry.io/contrib/config`. (#6289) ### Fixed - Transform nil attribute values to `log.Value` zero value instead of panicking in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6237) - Transform nil attribute values to `log.Value` zero value instead of panicking in `go.opentelemetry.io/contrib/bridges/otelzap`. (#6237) - Transform nil attribute values to `log.Value` zero value instead of `log.StringValue("<nil>")` in `go.opentelemetry.io/contrib/bridges/otelslog`. (#6246) - Fix `NewClientHandler` so that `rpc.client.request.*` metrics measure requests instead of responses and `rpc.client.responses.*` metrics measure responses instead of requests in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#6250) - Fix issue in `go.opentelemetry.io/contrib/config` causing `otelprom.WithResourceAsConstantLabels` configuration to not be respected. (#6260) - `otel.Handle` is no longer called on a successful shutdown of the Prometheus exporter in `go.opentelemetry.io/contrib/config`. (#6299)

m1heng requested review from dmathieu, pellared and a team as code owners October 12, 2024 08:15

m1heng force-pushed the feat/align-type branch 2 times, most recently from 7d3a3be to 6aa37ba Compare October 13, 2024 03:10

m1heng changed the title ~~Align logrus convertValue to logr~~ Move convert values to internal/shared/logutil Oct 13, 2024

m1heng mentioned this pull request Oct 13, 2024

Handle nil pointers #6236

Closed

m1heng force-pushed the feat/align-type branch from 6aa37ba to 74ac4bf Compare October 13, 2024 03:13

pellared reviewed Oct 13, 2024

View reviewed changes

internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved

internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved

internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved

m1heng force-pushed the feat/align-type branch 2 times, most recently from 98baa6a to a1c5708 Compare October 14, 2024 14:01

Move convert values to internal/shared/logutil

36b6507

m1heng force-pushed the feat/align-type branch from a1c5708 to 36b6507 Compare October 14, 2024 14:28

pellared added 2 commits October 14, 2024 18:19

Merge branch 'main' into feat/align-type

125cc8e

Update CHANGELOG.md

fc38d60

pellared changed the title ~~Move convert values to internal/shared/logutil~~ Fix panic for nil attributes and move convert values to internal/shared/logutil Oct 14, 2024

Update CHANGELOG.md

b9422f1

pellared changed the title ~~Fix panic for nil attributes and move convert values to internal/shared/logutil~~ Fix panic for nil attributes and move convert funcs to internal/shared/logutil Oct 14, 2024

pellared reviewed Oct 14, 2024

View reviewed changes

internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved

internal/shared/logutil/convert.go.tmpl Outdated Show resolved Hide resolved

Update internal/shared/logutil/convert.go.tmpl

9bca788

Co-authored-by: Robert Pająk <pellared@hotmail.com>

Update internal/shared/logutil/convert.go.tmpl

27601bc

Co-authored-by: Robert Pająk <pellared@hotmail.com>

m1heng force-pushed the feat/align-type branch from fa5f4ff to 27601bc Compare October 14, 2024 16:50

pellared approved these changes Oct 14, 2024

View reviewed changes

MrAlias approved these changes Oct 14, 2024

View reviewed changes

Merge branch 'main' into feat/align-type

4d88ad9

dmathieu approved these changes Oct 15, 2024

View reviewed changes

pellared added this to the v1.32.0 milestone Oct 15, 2024

Merge branch 'main' into feat/align-type

3f5847a

pellared merged commit 32c279a into open-telemetry:main Oct 15, 2024
24 checks passed

pellared mentioned this pull request Oct 15, 2024

oteslog: Improve transforming slog.KindAny attributes #6254

Merged

pellared added a commit that referenced this pull request Oct 16, 2024

oteslog: Improve transforming slog.KindAny attributes (#6254)

e1389ca

Reuse convertValue in otelslog. Follows #6237.

pellared mentioned this pull request Nov 8, 2024

Release v1.32.0/v0.57.0/v0.26.0/v0.12.0/v0.7.0/v0.5.0/v0.4.0 #6311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic for nil attributes and move convert funcs to internal/shared/logutil #6237

Fix panic for nil attributes and move convert funcs to internal/shared/logutil #6237

m1heng commented Oct 12, 2024 •

edited

Loading

pellared commented Oct 12, 2024 •

edited

Loading

m1heng commented Oct 13, 2024 •

edited

Loading

pellared left a comment

codecov bot commented Oct 13, 2024 •

edited

Loading

pellared commented Oct 13, 2024

m1heng commented Oct 14, 2024

m1heng commented Oct 14, 2024

pellared commented Oct 14, 2024 •

edited

Loading

m1heng commented Oct 14, 2024

pellared commented Oct 15, 2024

Fix panic for nil attributes and move convert funcs to internal/shared/logutil #6237

Fix panic for nil attributes and move convert funcs to internal/shared/logutil #6237

Conversation

m1heng commented Oct 12, 2024 • edited Loading

pellared commented Oct 12, 2024 • edited Loading

m1heng commented Oct 13, 2024 • edited Loading

pellared left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 13, 2024 • edited Loading

Codecov Report

pellared commented Oct 13, 2024

m1heng commented Oct 14, 2024

m1heng commented Oct 14, 2024

pellared commented Oct 14, 2024 • edited Loading

m1heng commented Oct 14, 2024

pellared commented Oct 15, 2024

m1heng commented Oct 12, 2024 •

edited

Loading

pellared commented Oct 12, 2024 •

edited

Loading

m1heng commented Oct 13, 2024 •

edited

Loading

codecov bot commented Oct 13, 2024 •

edited

Loading

pellared commented Oct 14, 2024 •

edited

Loading