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

Change Logger.emit to accept options for extensibility. #5771

Open
bogdandrutu opened this issue Sep 3, 2024 · 3 comments
Open

Change Logger.emit to accept options for extensibility. #5771

bogdandrutu opened this issue Sep 3, 2024 · 3 comments
Labels
area:logs Part of OpenTelemetry logs bug Something isn't working pkg:API Related to an API package question Further information is requested

Comments

@bogdandrutu
Copy link
Member

Description

Before making log package stable, consider to add options to the emit method to allow extensibility in the future.

Expected behavior

	// Emit emits a log record.
	//
	// The record may be held by the implementation. Callers should not mutate
	// the record after passed.
	//
	// Implementations of this method need to be safe for a user to call
	// concurrently.
	Emit(ctx context.Context, record Record, opts ...LoggerEmitOption)
@bogdandrutu bogdandrutu added the bug Something isn't working label Sep 3, 2024
@pellared
Copy link
Member

pellared commented Sep 4, 2024

From https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#loggeremit:

Emit can be extended by adding new exported fields to the Record struct.

Are there any issues with such approach?

@pellared pellared added question Further information is requested pkg:API Related to an API package area:logs Part of OpenTelemetry logs labels Sep 4, 2024
@bogdandrutu
Copy link
Member Author

I am not sure if that covers all possible extensions (we may say that this may require a name change), but also supporting "...options" is free if no options are provided correct? May not cost as to have it?

@pellared
Copy link
Member

pellared commented Sep 5, 2024

I am not sure if that covers all possible extensions

It would be helpful to come up even with an artificial example. For instance, we added options to NewSimpleProcessor for sake of future compatibility even though we do not need it right now.

but also supporting "...options" is free if no options are provided correct? May not cost as to have it?

I disagree that it is free. It expands the API surface which needs to be documented, which is discoverable. Future contributors may be not sure if new extensions should be added via Record or as EmitOption. Moreover, any helpers would also need to accept the options. I find that adding options is an unnecessary complexity and is against the principle of least astonishment (POLA).

At last, the https://go.dev/blog/module-compatibility suggests to add config structs or options as a way to keep the API backwards compatible (not both at the same time).

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 bug Something isn't working pkg:API Related to an API package question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants