opentelemetry-go icon indicating copy to clipboard operation
opentelemetry-go copied to clipboard

Change Logger.emit to accept options for extensibility.

Open bogdandrutu opened this issue 1 year ago • 3 comments

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 avatar Sep 03 '24 20:09 bogdandrutu

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 avatar Sep 04 '24 11:09 pellared

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?

bogdandrutu avatar Sep 04 '24 20:09 bogdandrutu

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).

pellared avatar Sep 05 '24 06:09 pellared

Emit does not accept "options" (meaning options pattern) as this would decrease the performance of calls that would use these options.

References:

  • https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#options-as-parameter-to-loggeremit
  • https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#loggeremit

If needed we can add methods to Record for extensibility.

Planning to close in about a week if there would be no response.

pellared avatar Feb 27 '25 20:02 pellared

Planning to close in about a week if there would be no response.

Closing after a month

pellared avatar Mar 27 '25 11:03 pellared