Skip to content
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.

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

Make log/logtest more convenient #6341

Open
pellared opened this issue Feb 19, 2025 · 0 comments · May be fixed by #6342
Open

Make log/logtest more convenient #6341

pellared opened this issue Feb 19, 2025 · 0 comments · May be fixed by #6342
Assignees
Labels
area:logs Part of OpenTelemetry logs enhancement New feature or request pkg:API Related to an API package

Comments

@pellared
Copy link
Member

pellared commented Feb 19, 2025

I never personally used log/logtest, but now I see that its usage is not user friendly.

See example test:

package main

import (
	"testing"

	"github.com/google/go-cmp/cmp"
	"github.com/google/go-cmp/cmp/cmpopts"
	"go.opentelemetry.io/contrib/bridges/otelslog"
	"go.opentelemetry.io/otel/attribute"
	"go.opentelemetry.io/otel/log"
	"go.opentelemetry.io/otel/log/logtest"
)

func TestOTelSlog(t *testing.T) {
	rec := logtest.NewRecorder()
	l := otelslog.NewLogger(t.Name(),
		otelslog.WithLoggerProvider(rec),
	)

	l.Info("Ping")
	l.Debug("Pong", "foo", "bar")

	got := rec.Result()

	r1 := log.Record{}
	r1.SetSeverity(log.SeverityInfo)
	r1.SetBody(log.StringValue("Ping"))

	r2 := log.Record{}
	r2.SetSeverity(log.SeverityDebug)
	r2.SetBody(log.StringValue("Pong"))
	r2.AddAttributes(log.String("foo", "bar"))

	want := []*logtest.ScopeRecords{
		{
			Name: t.Name(),
			Records: []logtest.EmittedRecord{
				{Record: r1},
				{Record: r2},
			},
		},
	}

	if diff := cmp.Diff(want, got,
		cmp.Comparer(func(a, b log.Value) bool {
			return a.Equal(b)
		}),
		cmpopts.IgnoreUnexported(logtest.EmittedRecord{}),
		cmpopts.IgnoreFields(log.Record{}, "timestamp"),
		cmp.AllowUnexported(attribute.Set{}, attribute.Distinct{}, log.Record{})); diff != "" {
		t.Errorf("Recorded records mismatch (-want +got):\n%s", diff)
	}
}

We missed this because we rely too much on testify which does a lot of magic under the hook.
However, we cannot assume that our users use testify.
Using IgnoreUnexported, AllowUnexported seems bad from go-cmp.

I think we should refactor to make it easier to use without any libraries and maybe even without go-cmp.

Instead of AssertRecordEqual we should make the types comparable or add Equal methods if necessary.

From https://google.github.io/styleguide/go/best-practices#leave-testing-to-the-test-function:

Assertion helpers are functions that check the correctness of a system and fail the test if an expectation is not met. Assertion helpers are not considered idiomatic in Go.

More: https://go.dev/wiki/TestComments#assert-libraries

I also have discovered that RecordFactory is not used anywhere.

We should also add some example in https://pkg.go.dev/go.opentelemetry.io/otel/log/logtest.

I want logtest to be easy to use by people who use:

  • testify
  • go-cmp
  • just the standard library
@pellared pellared added area:logs Part of OpenTelemetry logs enhancement New feature or request pkg:API Related to an API package labels Feb 19, 2025
@pellared pellared self-assigned this Feb 19, 2025
@pellared pellared changed the title log/logtest is not convinent to use Make log/logtest more convinent to use Feb 19, 2025
@pellared pellared moved this from Todo to In Progress in Go: Logs (GA) Feb 19, 2025
@pellared pellared changed the title Make log/logtest more convinent to use Make log/logtest more convinent Feb 19, 2025
@MrAlias MrAlias changed the title Make log/logtest more convinent Make log/logtest more convenient Feb 19, 2025
@pellared pellared linked a pull request Feb 19, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logs Part of OpenTelemetry logs enhancement New feature or request pkg:API Related to an API package
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

1 participant