log/logtest: Redesign #6342

pellared · 2025-02-19T21:45:53Z

Fixes #6341

Remove RecordFactory
- The function is not used anywhere.
Change the result type of Recorder.Result from []*ScopeRecords to Recording et al.
- The types represent better the data model. For instance it is a map of scope -> records. Previously each Logger (with e.g. with the same name) was producing a new ScopeRecords element.
Remove AssertRecordEqual
- The new types are easier to compare with go-cmp, testify and even standard library. Give the user feedom on how to compare.
- Add testable example on how to compare using go-cmp. This is probably going to our the way of testing log bridges and instrumentation libraries emitting log or event records. More log/logtest: Redesign #6342 (comment)
No Equal (59b557a)
- We do not need it as we can use go-cmp. However, it even works testify and reflect.DeepEqual (sic!).
- We can always consider adding it later if there would be a demand from user. Similarly a Compare could also be needed in case of bigger demand. However, it feels YAGNI.

Here are examples of how the current tests of log bridges can be improved:

dmathieu

This looks very nice.
I think it would make sense to see what changes will be required in one of the bridges?

log/logtest/recorder.go

log/go.mod

linux-foundation-easycla · 2025-03-05T21:21:15Z

The committers listed above are authorized under a signed CLA.

✅ login: pellared / name: Robert Pająk (578f9ab, 17d92c9, 96271e9, e14db7f, e056b95, a84ba46, e6b7de6, 030f8a7, 9d68e80, 1b0a436, c89f6c9, a4c181d, f26e543, 49486c6, c4ac836, 69a1b37, 748c27d, a470236, 58a907f, bf915ff, cc5d7aa, 59b557a, f8b7763, 178b0db, c77a09f, d8c692d, 5846107, 7443c45, 145ace3, 348b615, f522d00, 0ead039, 9f19f30, dff04ce, 28a6964, 16942f1, 24a6f58, e52793e, c42fc68, 9c4ddd7, 05acbb6, 6542559, 1486ad5, 9bca3f2, 4021498, 598170c, 04a737d, e0dcc39, 54d03f4, 3462546, 210fd18)

pellared · 2025-03-06T11:00:24Z

/easycla

pellared · 2025-03-06T11:01:24Z

@dmathieu, PTAL

pellared · 2025-03-06T14:12:49Z

@open-telemetry/go-maintainers, planning to merge on Monday my morning and start working on creating PRs in Contrib.

pellared · 2025-03-07T05:32:54Z

@open-telemetry/go-maintainers, planning to merge on Monday my morning and start working on creating PRs in Contrib.

This PR still needs an additional approval.

MrAlias · 2025-03-07T16:23:11Z

CHANGELOG.md

+  - Remove `ScopeRecords`, `EmittedRecord`, `RecordFactory` types.
+  - Remove `AssertRecordEqual` function.


These need to be moved to the Removed section of the changelog.

MrAlias · 2025-03-07T16:24:57Z

log/logtest/example_test.go

+	opts := []cmp.Option{
+		cmpopts.IgnoreFields(logtest.Record{}, "Context"),                         // Ignore Context.
+		cmpopts.IgnoreTypes(time.Time{}),                                          // Ignore Timestamps.
+		cmpopts.SortSlices(func(a, b log.KeyValue) bool { return a.Key < b.Key }), // Unordered compare of the key values.
+		cmpopts.EquateEmpty(),                                                     // Empty and nil collections are equal.
+	}


Please do not exemplify use of cmp. This project heavily uses testify/assert. We should match that with our example here.

This was done following #6342 (comment)

I've liked working with go's CMP library when i've used it. It is definitely very powerful, but does take a while to figure out which options to pass. It would be good to document recommended options for most instrumentation tests. I suspect they will generally be the same for most instrumentation libraries

The main reason I do not want to use testify is (from #6342 (comment)):

With testify the biggest issue you cannot force []log.KeyValue to sort slices before comparing (unordered collection equality). This can make the test too much "white-boxed" and can easy break during refactoring.

The example draft PRs use cmp:

[ForReview] Refactor otelzap tests opentelemetry-go-contrib#6839

Refactor logrus test to use new logtest API opentelemetry-go-contrib#6846

Thoughts?

Yeah, I'm not in favor of this. We extensively use testify/assert throughout this and our other projects. We need to design testing libraries for the things we use, not have our testing libraries dictate to us what testing packages we use.

It seems to me the goal of this change is to allow both, the use of testify and its avoidance.
Maybe the example/doc should show both.

There are already some tests which use cmp in places where testify is not an effective choice. See: https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-go%20github.com%2Fgoogle%2Fgo-cmp%2Fcmp&type=code

We have nothing in CONTRIBUTING.md telling that we cannot use cmp or what are our testing strategies/technologies. And cmp is a recommended by the Go team for comparing structures: https://go.dev/wiki/TestComments

At last, I think that the tests that use testify would still be benefit from this change. Next week, I will look into refactoring otelzap tests so that they use testify. For instance, I could use assert.ElementsMatch instead of assert.Equal for asserting record's attributes. However, it will not work correctly for "complex attributes". Yet, I think may be worth exploring.

Really?

Here are all the places we currently use testify in otel. Here are all the places we use it in contrib.

Here are the places we use cmp in otel, and in contrib

That is 6109 uses of testify to 58.

This is pretty clear that we use testify in these projects. I don't think you can fairly say there is confusion about what technology we use here. A developer who has worked in either of the projects would not be unclear about what is the commonly used tooling.

At last, I think that the tests that use testify would still be benefit from this change.

If that is the case it needs to be shown. You are exemplifying a different technology than is used throughout our projects without showing the value to our existing development practices.

If you plan to show how this is a benefit, I am all for it. I do not think this should be merged prior to that given that should be the primary design driver, not cmp.

If you would like, we can codify our defacto choice in using testify as our testing technology throughout our project in a policy. I have not opposition to that.

If we want to remove our use of cmp to make it even clearer that we have a strong preference for using testify in our testing I am also not opposed to that.

If that is the case it needs to be shown. You are exemplifying a different technology than is used throughout our projects without showing the value to our existing development practices.

If you plan to show how this is a benefit, I am all for it. I do not think this should be merged prior to that given that should be the primary design driver, not cmp.

Ok. Then this is what I will be working on next week. Thanks for your feedback 👍

MrAlias

It seems like this PR is not so much a PR about redesigning the logtest package as it is proposing we adopt a different testing strategy. This is an inappropriate way to achieve this type of change.

I appreciate refactoring this package as needed, but it needs to be centered on supporting the technologies we already use. If we want to undertake the monumental task of changing our testing technology that needs to be a proposal and project on itself.

MrAlias · 2025-03-07T18:17:18Z

log/logtest/example_test.go

+	opts := []cmp.Option{
+		cmpopts.IgnoreFields(logtest.Record{}, "Context"),                         // Ignore Context.
+		cmpopts.IgnoreTypes(time.Time{}),                                          // Ignore Timestamps.
+		cmpopts.SortSlices(func(a, b log.KeyValue) bool { return a.Key < b.Key }), // Unordered compare of the key values.
+		cmpopts.EquateEmpty(),                                                     // Empty and nil collections are equal.
+	}


Yeah, I'm not in favor of this. We extensively use testify/assert throughout this and our other projects. We need to design testing libraries for the things we use, not have our testing libraries dictate to us what testing packages we use.

XSAM

I personally prefer testify than cmp, if one line of testify code can finish the job (compare and return; exit and print the diff when failed)

This one line of testify code is easy to write (not much of a chance to write a typo) and easy to understand

assert.Equal(t, want, got)

than cmp.

if diff := cmp.Diff(want, got); diff != "" {
	t.Errorf("Recorded records mismatch (-want +got):\n%s", diff)
}

For cmp, reviewers need additional attention since there is a chance of having a typo like diff == "".

We usually have a lot of data that need to be compared, so the gap in clearance between these two libraries can only be bigger.

pellared · 2025-03-08T08:18:50Z

After feedback received by @MrAlias and @XSAM, I think that if we have bigger issues with testify then we could consider adding something like https://pkg.go.dev/go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest#AssertEqual. This would follow our existing patterns and would be a way mitigate the issues of testify that I mentioned before.

pellared · 2025-03-18T11:10:52Z

I am closing this PR in favor of:

[Proposal] [DoNotMerge] log/logtest: Redesign 2nd edition #6464

pellared added 5 commits February 19, 2025 22:45

log/logtest: Redesign

e52793e

Use Time.Equal

17d92c9

Refactor

030f8a7

Refactor

bf915ff

Better printing

c77a09f

pellared added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 19, 2025

pellared added 3 commits February 19, 2025 23:14

Fix grammar

a470236

Add examples

d8c692d

Refactor

49486c6

pellared changed the title ~~log/logtest: Redesign~~ [WIP] log/logtest: Redesign Feb 19, 2025

Better comment

c89f6c9

pellared changed the title ~~[WIP] log/logtest: Redesign~~ [Prototype] log/logtest: Redesign Feb 19, 2025

pellared marked this pull request as ready for review February 19, 2025 22:49

pellared requested review from MrAlias, XSAM, dashpole and dmathieu as code owners February 19, 2025 22:49

pellared added 5 commits February 19, 2025 23:52

Refactor

748c27d

Test attributes comparison

96271e9

Refactor

e6b7de6

Rename RecordedRecords to Recording

578f9ab

format time

9d68e80

dmathieu reviewed Feb 20, 2025

View reviewed changes

log/logtest/recorder.go Outdated Show resolved Hide resolved

pellared self-assigned this Feb 20, 2025

pellared added pkg:API Related to an API package area:logs Part of OpenTelemetry logs labels Feb 20, 2025

pellared added 4 commits February 20, 2025 17:33

Merge branch 'main' into logtest-redesign

e14db7f

Merge branch 'main' into logtest-redesign

e056b95

Refactor equals methods to functions

7443c45

Refine Result and add tests

6542559

Fix build and refactor

348b615

pellared commented Feb 26, 2025

View reviewed changes

log/go.mod Show resolved Hide resolved

pellared changed the title ~~log/logtest: Redesign~~ [DoNotMergeBeforeRelease] log/logtest: Redesign Feb 26, 2025

pellared changed the title ~~[DoNotMergeBeforeRelease] log/logtest: Redesign~~ log/logtest: Redesign Mar 5, 2025

Merge branch 'main' into logtest-redesign

e0dcc39

Merge branch 'main' into logtest-redesign

210fd18

Merge branch 'main' into logtest-redesign

54d03f4

dmathieu approved these changes Mar 6, 2025

View reviewed changes

MrAlias reviewed Mar 7, 2025

View reviewed changes

pellared added 2 commits March 7, 2025 18:40

Update CHANGELOG.md

3462546

Merge branch 'main' into logtest-redesign

598170c

pellared requested a review from MrAlias March 7, 2025 17:41

MrAlias requested changes Mar 7, 2025

View reviewed changes

XSAM reviewed Mar 8, 2025

View reviewed changes

pellared marked this pull request as draft March 10, 2025 22:26

pellared added 4 commits March 10, 2025 23:27

Merge branch 'main' into logtest-redesign

24a6f58

Merge branch 'main' into logtest-redesign

04a737d

Better way to compare context

9c4ddd7

Merge branch 'main' into logtest-redesign

f522d00

pellared mentioned this pull request Mar 18, 2025

[Proposal] [DoNotMerge] log/logtest: Redesign 2nd edition #6464

Open

pellared closed this Mar 18, 2025

github-project-automation bot moved this from In Progress to Done in Go: Logs (GA) Mar 18, 2025

pellared removed this from Go: Logs (GA) Mar 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log/logtest: Redesign #6342

log/logtest: Redesign #6342

pellared commented Feb 19, 2025 •

edited

Loading

dmathieu left a comment

linux-foundation-easycla bot commented Mar 5, 2025 •

edited

Loading

pellared commented Mar 6, 2025

pellared commented Mar 6, 2025

pellared commented Mar 6, 2025 •

edited

Loading

pellared commented Mar 7, 2025

MrAlias Mar 7, 2025

pellared Mar 7, 2025

MrAlias Mar 7, 2025

pellared Mar 7, 2025 •

edited

Loading

MrAlias Mar 7, 2025

dmathieu Mar 7, 2025

pellared Mar 7, 2025 •

edited

Loading

MrAlias Mar 7, 2025 •

edited

Loading

MrAlias Mar 7, 2025

MrAlias Mar 7, 2025

MrAlias Mar 7, 2025

pellared Mar 7, 2025

MrAlias left a comment

MrAlias Mar 7, 2025

XSAM left a comment •

edited

Loading

pellared commented Mar 8, 2025 •

edited

Loading

pellared commented Mar 18, 2025

		- Remove `ScopeRecords`, `EmittedRecord`, `RecordFactory` types.
		- Remove `AssertRecordEqual` function.

log/logtest: Redesign #6342

log/logtest: Redesign #6342

Conversation

pellared commented Feb 19, 2025 • edited Loading

dmathieu left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Mar 5, 2025 • edited Loading

pellared commented Mar 6, 2025

pellared commented Mar 6, 2025

pellared commented Mar 6, 2025 • edited Loading

pellared commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

MrAlias Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrAlias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XSAM left a comment • edited Loading

Choose a reason for hiding this comment

pellared commented Mar 8, 2025 • edited Loading

pellared commented Mar 18, 2025

pellared commented Feb 19, 2025 •

edited

Loading

linux-foundation-easycla bot commented Mar 5, 2025 •

edited

Loading

pellared commented Mar 6, 2025 •

edited

Loading

pellared Mar 7, 2025 •

edited

Loading

pellared Mar 7, 2025 •

edited

Loading

MrAlias Mar 7, 2025 •

edited

Loading

XSAM left a comment •

edited

Loading

pellared commented Mar 8, 2025 •

edited

Loading