E5FF log: Fix comparison of unordered map values by dmathieu · Pull Request #5306 · open-telemetry/opentelemetry-go · GitHub
[go: up one dir, main page]
Skip to content

log: Fix comparison of unordered map values #5306

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
May 15, 2024
Merged

log: Fix comparison of unordered map values #5306

merged 8 commits into from
May 15, 2024

Conversation

dmathieu
Copy link
Member
@dmathieu dmathieu commented May 7, 2024 •
edited
Loading

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 Codecov
Copy link
codecov bot commented May 7, 2024 •
edited
Loading

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

Impacted file tree graph

@@          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

@dmathieu dmathieu marked this pull request as ready for review May 7, 2024 08:25
pellared
pellared requested changes May 7, 2024
View reviewed changes
Copy link
Member
@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dmathieu
Copy link
Member Author
dmathieu commented May 7, 2024

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

@MrAlias
Copy link
Contributor
MrAlias commented May 7, 2024

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
Copy link
Member
pellared commented May 7, 2024 •
edited
Loading

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
Copy link
Member Author
dmathieu commented May 7, 2024

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

@dmathieu dmathieu changed the title Fix comparison of unordered log attributes Fix comparison of unordered log map attributes May 7, 2024
@dmathieu dmathieu requested a review from pellared May 7, 2024 15:24
MadVikingGod
MadVikingGod suggested changes May 7, 2024
View reviewed changes
@dmathieu dmathieu force-pushed the log-keyvalue-sorted-equal branch from 482ba99 to d34abd9 Compare May 8, 2024 08:27
@dmathieu dmathieu requested a review from MadVikingGod May 8, 2024 08:27
@pellared
Copy link
Member
pellared commented May 8, 2024

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

@dmathieu
Copy link
Member Author
dmathieu commented May 8, 2024

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

pellared
pellared approved these changes May 8, 2024
View reviewed changes
MrAlias
MrAlias reviewed May 8, 2024
View reviewed changes
@MrAlias MrAlias added pkg:API Related to an API package area:logs Part of OpenTelemetry logs labels May 9, 2024
@MrAlias MrAlias added this to the v1.27.0 milestone May 9, 2024
@MrAlias
Copy link
Contributor
MrAlias commented May 9, 2024

@MadVikingGod PTAL

@pellared pellared mentioned this pull request May 10, 2024
Merged
MrAlias
MrAlias reviewed May 14, 2024
View reviewed changes
@dmathieu dmathieu force-pushed the log-keyvalue-sorted-equal branch from 49977a0 to 4e2e11e Compare May 15, 2024 07:20
@pellared pellared changed the title Fix comparison of unordered log map attributes Fix comparison of unordered log map values May 15, 2024
@pellared pellared CC27 changed the title Fix comparison of unordered log map values log: Fix comparison of unordered map values May 15, 2024
pellared
pellared reviewed May 15, 2024
View reviewed changes
@pellared pellared merged commit 08c8b32 into open-telemetry:main May 15, 2024
@MrAlias MrAlias mentioned this pull request May 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@MrAlias MrAlias MrAlias left review comments

@XSAM XSAM XSAM approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@dashpole dashpole Awaiting requested review from dashpole

+2 more reviewers

@MadVikingGod MadVikingGod MadVikingGod approved these changes

@hanyuancheung hanyuancheung hanyuancheung approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area:logs Part of OpenTelemetry logs pkg:API Related to an API package
Milestone
v1.27.0
0