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

log/logtest: Add log.Record initializer #5195

Closed
dmathieu opened this issue Apr 11, 2024 · 5 comments · Fixed by #5263
Closed

log/logtest: Add log.Record initializer #5195

dmathieu opened this issue Apr 11, 2024 · 5 comments · Fixed by #5263
Assignees
Labels
area:logs Part of OpenTelemetry logs enhancement New feature or request pkg:API Related to an API package
Milestone

Comments

@dmathieu
Copy link
Member

Problem Statement

Right now, creating and filling data for a log.Record requires calling method independently, and cannot be chained.

eg:

var record log.Record
record.SetTimestamp(e.Time)
record.SetBody(log.StringValue(e.Message))
record.SetSeverity(severity)
record.AddAttributes(data...)

This is a bit tedious, especially in test environments, where we end up having to setup methods to build records such as this one in the logrus bridge PR:

func buildRecord(body log.Value, timestamp time.Time, severity log.Severity, attrs []log.KeyValue) log.Record {
	var record log.Record
	record.SetBody(body)
	record.SetTimestamp(timestamp)
	record.SetSeverity(severity)
	record.AddAttributes(attrs...)

	return record
}

Proposed Solution

Add a NewRecord() helper method with optional arguments to create a new record.
The example above would become:

record := NewRecord(
    WithTimestamp(e.Time),
    WithBody(message),
    WithSeverity(severity),
    WithAttributes(data),
)

Alternative

I could change my buildRecord in logrus to use functional arguments, but that would have to be repeated across all bridges.

@dmathieu dmathieu added enhancement New feature or request area:logs Part of OpenTelemetry logs labels Apr 11, 2024
@pellared
Copy link
Member

pellared commented Apr 11, 2024

The functional options would introduce heap allocations. I do not support the proposal as we would then provide inefficient API that developers might use and then compliant that the API is not performant. More: https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#options-as-parameter-to-loggeremit.

@dmathieu
Copy link
Member Author

What if we add it to the logtest package?
Then writing tests for bridges/implementations becomes easier, and there's a clear indication that it shouldn't be used at runtime?

@pellared
Copy link
Member

pellared commented Apr 11, 2024

What if we add it to the logtest package? Then writing tests for bridges/implementations becomes easier, and there's a clear indication that it shouldn't be used at runtime?

That makes a lot of sense.

I would also consider implementing it as a "builder" to not "spam" the package with options. So that the user could do something like:

record := logtest.NewRecordBuilder().
  SetBody(log.StringValue("message").
  SetTimestamp(timestamp).
  SetSeverity(severity).
  AddAttributes(data).
  Build()

It might be possible that a builder would not make a heap allocation - however, I doubt it.

@pellared pellared self-assigned this Apr 17, 2024
@pellared pellared added the pkg:API Related to an API package label Apr 17, 2024
@pellared pellared changed the title Add log.Record initializer log/logtest: Add log.Record initializer Apr 17, 2024
@pellared
Copy link
Member

@dmathieu, do you think we can do something like #5234 for this issue as well?

@dmathieu
Copy link
Member Author

Yes, definitely. I'll wait for your PR to be approved before I start doing something similar for the API record.

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: Done
Development

Successfully merging a pull request may close this issue.

3 participants