Proposal: More flexible Logger.emit

[agagniere]

Superseeds #154

#154 is non-breaking but adds a new method for emitting logs, that is the same as emit but with an additional argument.

This PR proposes a more future-proof solution, with an Option struct argument with default value, allowing optional arguments for emit.

This will allow, for instance, adding source location, without adding yet another method, and without breaking existing calls.

The proposed new signature is:

• mandatory severity level: Pass an explicit 0 for no level optional severity
  • can be null for an unspecified level
  • either a level name: .info, .err, etc
  • or a severity number: .{ .severity = 8 }
• mandatory body string (see Proposal: structured logs #158 for the structured body use case)
• the rest goes in the option struct and are all optional:
  • timestamp
  • observed timestamp. If unspecified, the current wall time will be used
  • severity text (in general: redundant with the number while taking more space on the wire)
  • attributes
  • span context, for trace correlation

[inge4pres]

Can you please add all optional parameters dicated by the API spec of emit?
https://opentelemetry.io/docs/specs/otel/logs/api/#emit-a-logrecord

I like the dedicated struct for options, it's also wide-spread practice in Zig codebases, so we can have all fields in the struct.

[kmos]

I suggest to add for now Timestamp and Observed Timestamp which are actually optional and already supported by ReadWriteLogRecord. If I recall correctly we support implicit Context and we can later on support an explicit definition.

[inge4pres]

By reading this PR I realized our existing implementation of emit does not satisfy the API specification.
I don't remember why @kmos and I didn't add them in the first place when developing the logsAPI, but this looks like a great time to add them 👍🏼

[kmos]

Thanks @agagniere for the proposal. Actually the SDK is under heavy development and I don't see any issue to have breaking changes (@inge4pres do you agree?).

I have a couple of points:

1. mandatory severity level: Pass an explicit 0 for no level: Why don't we keep it optional like API spec and in case of null we set it to 0?
2. mandatory body string: giving a look to other SDKs is completely aligned 👍
3. severity text (in general: redundant with the number while taking more space on the wire): I agree to move it inside a struct. I think that we could have the logic defined in the spec: If the text is not specified, we should use the short text assigned to the severity number. It can be done in another PR (?) @inge4pres
4. span context, for trace correlation: I am a bit confused on this point. is it part of the spec the span context for correlation? @inge4pres @agagniere

By reading this PR I realized our existing implementation of emit does not satisfy the API specification.
I don't remember why @kmos and I didn't add them in the first place when developing the logsAPI, but this looks like a great time to add them 👍🏼

Yeah. I think because we were really focused on having a complete "flow". My real concern is the missing (optional) event name.

[inge4pres]

LGTM, nice improvement of the API, thanks 🙏🏼
non blocking questions down here

[agagniere]

I'll add another commit to pass a Context, then you can squash.

Not-yet written follow-up PRs:

• using the current context if not explicitely provided
• Add event name arg & export it in OTLP

[inge4pres]

I'll add another commit to pass a Context,

Thanks, I completely slipped in this during the review, indeed we should use Context there.

[inge4pres]

Wonderful 👏🏼 merging this one now

[inge4pres]

Nice addition 👍🏼