log: Fix comparison of unordered map values #5306

dmathieu · 2024-05-07T08:20:53Z

Map attributes should be equal even if their order is different, which can cause issues, for example in open-telemetry/opentelemetry-go-contrib#5356.

goarch: arm64
pkg: go.opentelemetry.io/otel/log
                                                                               │     bench-main     │             bench-branch              │
                                                                               │       sec/op       │       sec/op        vs base           │
ValueEqual/[l:[3_foo]_b:[3_5_7]_e:<nil>]_with_[l:[3_foo]_b:[3_5_7]_e:<nil>]-10   0.000001100n ± ∞ ¹   0.000001700n ± ∞ ¹  ~ (p=1.000 n=1) ²
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                                                                               │ bench-main  │          bench-branch          │
                                                                               │    B/op     │    B/op      vs base           │
ValueEqual/[l:[3_foo]_b:[3_5_7]_e:<nil>]_with_[l:[3_foo]_b:[3_5_7]_e:<nil>]-10   0.000 ± ∞ ¹   0.000 ± ∞ ¹  ~ (p=1.000 n=1) ²
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal

                                                                               │ bench-main  │          bench-branch          │
                                                                               │  allocs/op  │  allocs/op   vs base           │
ValueEqual/[l:[3_foo]_b:[3_5_7]_e:<nil>]_with_[l:[3_foo]_b:[3_5_7]_e:<nil>]-10   0.000 ± ∞ ¹   0.000 ± ∞ ¹  ~ (p=1.000 n=1) ²
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal

codecov · 2024-05-07T08:24:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.6%. Comparing base (56bb4cf) to head (5397061).

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5306   +/-   ##
=====================================
  Coverage   84.6%   84.6%           
=====================================
  Files        268     268           
  Lines      17695   17704    +9     
=====================================
+ Hits       14971   14981   +10     
+ Misses      2412    2411    -1     
  Partials     312     312

Files	Coverage Δ
log/keyvalue.go	`96.8% <100.0%> (+0.1%)`	⬆️

... and 2 files with indirect coverage changes

pellared

While I agree that for maps the order should not make any difference, I do not think the same applies for slices.

dmathieu · 2024-05-07T12:04:20Z

My linked PR for logrus has different orders based on the OS (my mac, and linux on CI), which caused failures.

MrAlias · 2024-05-07T14:48:03Z

My linked PR for logrus has different orders based on the OS (my mac, and linux on CI), which caused failures.

Can you sort your slice values before creating the attribute?

pellared · 2024-05-07T14:50:35Z

Can you sort your slice values before creating the attribute?

Or sort in tests if for logrus the order is not preserved.

PS. Maybe https://pkg.go.dev/github.com/stretchr/testify/assert#ElementsMatch will help.

dmathieu · 2024-05-07T15:24:09Z

Actually, my failing logrus test is because of a map, not a slice. So I have removed slice sorting from here.
https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5356/files#diff-f54db292144cf7bd4ae4be9b3f4b703416a1352cf27304bca223d8cf00c90dc5R339-R345

log/keyvalue.go

pellared · 2024-05-08T08:28:33Z

Can you update the benchmark results? I believe they will be different 😉

dmathieu · 2024-05-08T08:35:44Z

I have updated them (with only map->map comparison to ease reading). Surprisingly, the difference is very small.

log/keyvalue.go

CHANGELOG.md

log/keyvalue_test.go

MrAlias · 2024-05-09T21:15:33Z

@MadVikingGod PTAL

CHANGELOG.md

This release includes the first beta release of the OpenTelemetry Logs Bridge API and SDK for Go. ### Added - Add example for `go.opentelemetry.io/otel/exporters/stdout/stdoutlog`. (#5242) - Add `RecordFactory` in `go.opentelemetry.io/otel/sdk/log/logtest` to facilitate testing exporter and processor implementations. (#5258) - Add `RecordFactory` in `go.opentelemetry.io/otel/log/logtest` to facilitate testing bridge implementations. (#5263) - The count of dropped records from the `BatchProcessor` in `go.opentelemetry.io/otel/sdk/log` is logged. (#5276) - Add metrics in the `otel-collector` example. (#5283) - Add the synchronous gauge instrument to `go.opentelemetry.io/otel/metric`. (#5304) - An `int64` or `float64` synchronous gauge instrument can now be created from a `Meter`. - All implementations of the API (`go.opentelemetry.io/otel/metric/noop`, `go.opentelemetry.io/otel/sdk/metric`) are updated to support this instrument. - Add logs to `go.opentelemetry.io/otel/example/dice`. (#5349) ### Changed - The `Shutdown` method of `Exporter` in `go.opentelemetry.io/otel/exporters/stdout/stdouttrace` ignores the context cancellation and always returns `nil`. (#5189) - The `ForceFlush` and `Shutdown` methods of the exporter returned by `New` in `go.opentelemetry.io/otel/exporters/stdout/stdoutmetric` ignore the context cancellation and always return `nil`. (#5189) - Apply the value length limits to `Record` attributes in `go.opentelemetry.io/otel/sdk/log`. (#5230) - De-duplicate map attributes added to a `Record` in `go.opentelemetry.io/otel/sdk/log`. (#5230) - `go.opentelemetry.io/otel/exporters/stdout/stdoutlog` won't print timestamps when `WithoutTimestamps` option is set. (#5241) - The `go.opentelemetry.io/otel/exporters/stdout/stdoutlog` exporter won't print `AttributeValueLengthLimit` and `AttributeCountLimit` fields now, instead it prints the `DroppedAttributes` field. (#5272) - Improved performance in the `Stringer` implementation of `go.opentelemetry.io/otel/baggage.Member` by reducing the number of allocations. (#5286) - Set the start time for last-value aggregates in `go.opentelemetry.io/otel/sdk/metric`. (#5305) - The `Span` in `go.opentelemetry.io/otel/sdk/trace` will record links without span context if either non-empty `TraceState` or attributes are provided. (#5315) - Upgrade all dependencies of `go.opentelemetry.io/otel/semconv/v1.24.0` to `go.opentelemetry.io/otel/semconv/v1.25.0`. (#5374) ### Fixed - Comparison of unordered maps for `go.opentelemetry.io/otel/log.KeyValue` and `go.opentelemetry.io/otel/log.Value`. (#5306) - Fix the empty output of `go.opentelemetry.io/otel/log.Value` in `go.opentelemetry.io/otel/exporters/stdout/stdoutlog`. (#5311) - Split the behavior of `Recorder` in `go.opentelemetry.io/otel/log/logtest` so it behaves as a `LoggerProvider` only. (#5365) - Fix wrong package name of the error message when parsing endpoint URL in `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#5371) - Identify the `Logger` returned from the global `LoggerProvider` in `go.opentelemetry.io/otel/log/global` with its schema URL. (#5375)

dmathieu marked this pull request as ready for review May 7, 2024 08:25

dmathieu requested review from MrAlias, Aneurysm9, evantorrie, XSAM, dashpole, MadVikingGod, pellared and hanyuancheung as code owners May 7, 2024 08:25

pellared requested changes May 7, 2024

View reviewed changes

dmathieu changed the title ~~Fix comparison of unordered log attributes~~ Fix comparison of unordered log map attributes May 7, 2024

dmathieu requested a review from pellared May 7, 2024 15:24

MadVikingGod suggested changes May 7, 2024

View reviewed changes

log/keyvalue.go Outdated Show resolved Hide resolved

dmathieu force-pushed the log-keyvalue-sorted-equal branch from 482ba99 to d34abd9 Compare May 8, 2024 08:27

dmathieu requested a review from MadVikingGod May 8, 2024 08:27

pellared approved these changes May 8, 2024

View reviewed changes

log/keyvalue.go Show resolved Hide resolved

MrAlias reviewed May 8, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

log/keyvalue_test.go Show resolved Hide resolved

XSAM approved these changes May 9, 2024

View reviewed changes

MrAlias added pkg:API Related to an API package area:logs Part of OpenTelemetry logs labels May 9, 2024

MrAlias added this to the v1.27.0 milestone May 9, 2024

MadVikingGod approved these changes May 9, 2024

View reviewed changes

pellared mentioned this pull request May 10, 2024

logrus logging bridge open-telemetry/opentelemetry-go-contrib#5356

Merged

hanyuancheung approved these changes May 12, 2024

View reviewed changes

MrAlias reviewed May 14, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

dmathieu added 7 commits May 15, 2024 09:19

sort slices and maps key/values when comparing them

3ef32e1

add changelog entry

a9f2f4f

add benchmark value equal

3f662d4

pr number in changelog

92e789f

only sort maps, not slices

5a94996

don't mutate the map when comparing it

ed4bb0e

fix changelog entry

4e2e11e

dmathieu force-pushed the log-keyvalue-sorted-equal branch from 49977a0 to 4e2e11e Compare May 15, 2024 07:20

pellared changed the title ~~Fix comparison of unordered log map attributes~~ Fix comparison of unordered log map values May 15, 2024

pellared changed the title ~~Fix comparison of unordered log map values~~ log: Fix comparison of unordered map values May 15, 2024

pellared reviewed May 15, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

5397061

pellared merged commit 08c8b32 into open-telemetry:main May 15, 2024
26 checks passed

MrAlias mentioned this pull request May 21, 2024

Release v1.27.0/v0.49.0/v0.3.0 #5392

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log: Fix comparison of unordered map values #5306

log: Fix comparison of unordered map values #5306

dmathieu commented May 7, 2024 •

edited

Loading

codecov bot commented May 7, 2024 •

edited

Loading

pellared left a comment

dmathieu commented May 7, 2024

MrAlias commented May 7, 2024

pellared commented May 7, 2024 •

edited

Loading

dmathieu commented May 7, 2024

pellared commented May 8, 2024

dmathieu commented May 8, 2024

MrAlias commented May 9, 2024

log: Fix comparison of unordered map values #5306

log: Fix comparison of unordered map values #5306

Conversation

dmathieu commented May 7, 2024 • edited Loading

codecov bot commented May 7, 2024 • edited Loading

Codecov Report

pellared left a comment

Choose a reason for hiding this comment

dmathieu commented May 7, 2024

MrAlias commented May 7, 2024

pellared commented May 7, 2024 • edited Loading

dmathieu commented May 7, 2024

pellared commented May 8, 2024

dmathieu commented May 8, 2024

MrAlias commented May 9, 2024

dmathieu commented May 7, 2024 •

edited

Loading

codecov bot commented May 7, 2024 •

edited

Loading

pellared commented May 7, 2024 •

edited

Loading