search

open-telemetry / opentelemetry-go

OpenTelemetry Go API and SDK
https://opentelemetry.io/docs/languages/go
Apache License 2.0
5.19k stars 1.04k forks source link

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

Open bogdandrutu opened 3 weeks ago

bogdandrutu commented 3 weeks ago

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)
pellared commented 3 weeks ago

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?

bogdandrutu commented 3 weeks ago

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 commented 3 weeks ago

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