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

Add EventName to Log and Event Record in data model #4260

Closed
pellared opened this issue Oct 15, 2024 · 8 comments · Fixed by #4320
Closed

Add EventName to Log and Event Record in data model #4260

pellared opened this issue Oct 15, 2024 · 8 comments · Fixed by #4320
Assignees
Labels
area:data-model For issues related to data model sig-issue A specific SIG should look into this before discussing at the spec spec:logs Related to the specification/logs directory

Comments

@pellared
Copy link
Member

pellared commented Oct 15, 2024

This is a proposal to add EventName of type string as a field in the data model instead of having it just a event.name string attribute defined in semantic conventions.

Related to:

Consequences

  • Currently, we have no control what type of value would be set to event.name (e.g. by log bridges). Promoting it to a field with a concrete type would make the design more type-safe. So far my understanding is that most people would like to have it as string. On the other hand, there are some cases where users would like to have something different or more lightweight. Personally, I think settling on a concrete "type" is better than having something very generic and abstract. I think that having EventId of any type is nothing better than having event.name defined in semantic conventions. I would consider having EventId of an integer type (instead of EventName as string) for performance reasons; yet this can be tracked separate issue.

It is very important (actually the EventId, the numerical, machine friendly version of EventName that is most important to do ultra fast checks!) for scenarios we are working on in OTel Rust, C++. I don't know if it is something every language/implementation cares about. Given spec does not prohibit an implementation from allowing more parameters, I am totally okay if spec does not mention it, as OTel C++/Rust can offer this as extras.

Originally posted by @cijothomas in #4203 (comment)

  • Having event name as a field would make adding some events processing functionalities to the Logs SDK easier (e.g. filtering based on event name)

I am against this proposal as I do not think that handling event.name for Enabled is more important than for other attributes. Passing all attributes "destroys" the idea of Enabled which is supposed to be used by the user to check whether it makes sense to build and emit a log records (performance tuning). The parameters accepted by Logger.Enabled should be "cheap" to construct.

The SDK's Enabled implementation would get the context, severity level, instrumentation scope that should be enough.

Side note: I think that instrumentation libraries emitting events could add a event.namespace instrumentation scope attribute instead of putting the namespace to each event.name value as a prefix. EDIT: I created #4239

If "event.name" is an attribute, then I totally agree!
(Which is why OTel .NET, C++, Rust all decided to put EventName as a top-level field in LogRecord, and not inside Attributes)

Originally posted by @cijothomas in #4220 (comment)

  • Bridges for logging libraries that do not have a concept of log record identifier would need to have some convention to map event.name attribute into EventName field. This mapping could be configurable. Therefore, people who do not want to emit events using logging libraries may disable this mapping behavior.

  • Emitting events would need to be done by calling Emit Event on Logger (see Add EmitEvent to Logs API #4259) (unless optional parameter EventName is added to Logger.Emit). Log bridges would have to call Emit Event when they would want to produce events

Remark

The main question is whether we want to promote the event.name attribute to a LogRecord EventName string field. The decision on the desired field's name and type can be tracked as a separate issue.

@pellared pellared changed the title [Logs] Add EventName to LogRecord data model [logs] Add EventName to LogRecord data model Oct 15, 2024
@pellared pellared added spec:logs Related to the specification/logs directory area:data-model For issues related to data model labels Oct 15, 2024
@pellared pellared changed the title [logs] Add EventName to LogRecord data model Add EventName to LogRecord data model Oct 15, 2024
@pellared pellared changed the title Add EventName to LogRecord data model Add EventName or EventId to LogRecord data model Oct 15, 2024
@pellared pellared changed the title Add EventName or EventId to LogRecord data model Add EventName to LogRecord data model Oct 15, 2024
@pellared pellared changed the title Add EventName to LogRecord data model Add EventName to Log and Event Record in data model Oct 15, 2024
@breedx-splk breedx-splk moved this to Todo in Logs SIG Oct 15, 2024
@trask trask added the sig-issue A specific SIG should look into this before discussing at the spec label Oct 15, 2024
@pellared
Copy link
Member Author

pellared commented Oct 16, 2024

I think this change is important from design perspective, if we want to make sure that we have separate APIs for emitting log records and events.

It allows as to make it impossible to emit an event using Emit a LogRecord - assuming we will not add an optional EventName parameter.

The events would have to be emitted using Emit an Event by both instrumentation code (applications, instrumentation libraries) and bridges (it should be possible as marked in OTEP - maybe it could an opt-in functionality of a bridge) and the parameters could be more constraint than the one for Emit a Record e.g.:

  • SeverityNumber is required
  • Attributes as "standard attributes" type (instead of log attributes)

@tedsuo
Copy link
Contributor

tedsuo commented Oct 22, 2024

I would be in favor of this!

I have concerns around supporting 3rd party loggers wanting to create events. Those loggers would not have a name field, so they would need to use something like an event.name attribute. As long as we think these events can be caught and transformed properly before entering into any Otel pipeline then this is not a problem. If we cannot have this expectation, then any pipeline processor would have to check two places for the event name – the event.name attribute and the Name field. That would be annoying, I would prefer we avoid this scenario.

@tedsuo
Copy link
Contributor

tedsuo commented Oct 22, 2024

But I do think the Name field should be added as an option onto the EmitLogRecord function. In part for the reasons described in the comment above: there may be bridges to loggers that are creating events in some way. But also because it seems wrong to hide it.

@pellared
Copy link
Member Author

pellared commented Oct 22, 2024

there may be bridges to loggers that are creating events in some way

The bridges can use Emit a Record when they want to emit an event and not a regular log record. See #4260 (comment)

@pellared
Copy link
Member Author

SIG meeting notes:

  • This should be blocking stabilization of events handling in Logs SDK.
  • All attendees are in favor of this change but it would have to be validated with the rest of the OTel community.

@lmolkova
Copy link
Contributor

lmolkova commented Nov 28, 2024

Based on the discussion in the Logs SIG (11/26) and during spec call (11/19) we'd like to proceed with the change.

We'll break it down into several pieces

  1. Update data model in the spec repo -Change event.name attribute into top-level event name field #4320

    • Update Logger.emit or LogRecord API to take optional event name
  2. Update proto definition - Add event_name to logs proto opentelemetry-proto#600

  3. Deprecate event.name attribute in semantic conventions and update links to data model. - @lmolkova

  4. PRs/prototypes for OTel API/SDK that add EventName

    • Python + OpenAI instrumentation with events - @lmolkova
    • Java - @trask
    • Collector - @lmolkova
      - OTLP receiver/exporter
      - Processors - anything special there?
  5. Socialize it across sigs (spec, maintainers, client, semconv)

There will be no backward compatibility or transition period given that event.name is an experimental attribute used by relatively new experimental conventions.

  • OTel SDKs that provide emit event API should populate top-level EventName property on the LogRecord instead of event.name attribute when this attribute was populated by the SDK.
  • If event.name is provided by the caller, SDK is not expected to detect or remove it.
  • Same goes to collector: it is not expected to interact with event.name attribute in any specific way. Once new proto version is supported, users should be able to use transformation processor to interact with EventName and potentially populate it using event.name attribute value.
  • Instrumentations that produce events using explicit event.name attribute can do a breaking change and start using LogRecord.EventName property instead of the attribute. It's up to each instrumentation component to provide (or not provide) transition story.
  • Telemetry consumers should expect to receive deprecated event.name attribute, new LogRecord.EventName property or both. When both are present, the LogRecord.EventName should be used as a source of truth.

@lmolkova
Copy link
Contributor

Semantic conventions that depend on event.name:

/cc @open-telemetry/semconv-mobile-approvers @open-telemetry/semconv-feature-flag-approvers, LMK if I missed something and there are instrumentations or other components that populate event.name attribute.

@tigrannajaryan
Copy link
Member

There will be no backward compatibility or transition period given that event.name is an experimental attribute used by relatively new experimental conventions.

I like the simplicity of this approach, however in the spec call where this change was discussed the idea that we will have a transition period was floated. Please make sure there is clear visibility that we are not doing it (Slack, spec call, Log SIG, etc). I want to make sure there are no existing users of event.name that we don't know about and who we will cause pain by this decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:data-model For issues related to data model sig-issue A specific SIG should look into this before discussing at the spec spec:logs Related to the specification/logs directory
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants