open-telemetry / oteps

OpenTelemetry Enhancement Proposals
https://opentelemetry.io
Apache License 2.0
337 stars 164 forks source link

Events and Logs API #202

Closed scheler closed 2 years ago

scheler commented 2 years ago

We introduce an Events and Logs API that is based on the OpenTelemetry Log signal, backed by LogRecord data model and LogEmitter SDK. This is meant to be used by end-users for creating Events and by the Log Appender authors to create Logs.

linux-foundation-easycla[bot] commented 2 years ago

CLA Signed

The committers listed above are authorized under a signed CLA.

MSNev commented 2 years ago

Related issue, which could allow for having a common EventsApi for using Logs or Traces, if the Attributes definition is expanded to match the protocol. https://github.com/open-telemetry/opentelemetry-specification/issues/2581

bogdandrutu commented 2 years ago

As logs approvers, I would like more opinions:

@kumoroku @djaglowski @zenmoto

tigrannajaryan commented 2 years ago

All, Log SIG has been discussing this initiative for the last few weeks. If you want to talk about this live please join the Log SIG meeting today.

tigrannajaryan commented 2 years ago

All, we had an extensive discussion of this OTEP in Log SIG. There is one major blocker which prevents us from moving forward: whether there needs to be a single Logs/Events API or 2 separate APIs.

I started a document to compare the 2 approaches (separate vs one API) side-by-side: https://docs.google.com/document/d/1Uy5L3UThFyc4PelgH51G3OCLFbvzq2xWiNDy2G8iXxU/edit# The document also has the links to prototypes with both approaches.

We agreed in the Log SIG we are going to make the choice by the next Log SIG (next Wed). If you have any opinion on this topic please add arguments in favour of the one of the approach to the document.

tigrannajaryan commented 2 years ago

@scheler to clarify, you are going with this, right? image

scheler commented 2 years ago

Yes, but the logEvent() method is on the Logger and not on EventBuilder. The EventBuilder is obtained from Logger as Logger.eventBuilder("myEventName"), and so the EventBuilder returned will have the event.name attribute propulated in the LogRecord. Further, the EventBuilder will only have setters for various properties of LogRecord and will not need the logEvent method. The logEvent method is a convenience method in the Logger class, to create the Event without using the builder pattern.

scheler commented 2 years ago

@tigrannajaryan I updated the OTEP with all the discussed changes, can you take a look?

tigrannajaryan commented 2 years ago

Generally LGTM. This aligns well with what we discussed in the Log SIG. I left some mostly minor comments.

scheler commented 2 years ago

It makes sense, I will make the changes.

On Thu, Jun 23, 2022, 1:50 AM Daniel Dyla @.***> wrote:

@.**** commented on this pull request.

In text/0202-events-and-logs-api.md https://github.com/open-telemetry/oteps/pull/202#discussion_r904191572:

+public void addBrowserEvent(String name, Attributes attributes) {

  • Logger logger = openTelemetry.getLogger("my-scope", "1.0", "browser");
  • logger.logEvent(name, attributes); +}
  • +public void addMobileEvent(String name, Attributes attributes) {

  • Logger logger = openTelemetry.getLogger("my-scope", "1.0", "mobile");
  • logger.logEvent(name, attributes); +} +```
  • +### Usage in eBPF

  • +From the eBPF demo, it looks like they have chosen to put the Event data in the Body field of LogRecord instead of Attributes. They might have to make the changes to move the event data to attributes to conform to this API specification.

  • +## Semantic Convention for event attributes

I still think this whole section seems like an implementation detail the spec should handle. The OTEP should make an argument for why events should have a required name and define and argue for the existence of a domain, but specific key names for semconv is an implementation detail to be handled by the spec.

This is just my opinion so if you don't agree fine, I don't view it as a blocker.

— Reply to this email directly, view it on GitHub https://github.com/open-telemetry/oteps/pull/202#discussion_r904191572, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVMH2GJ6HFBS5C3GW2Q72LVQNYP5ANCNFSM5VTMMZQQ . You are receiving this because you were mentioned.Message ID: @.***>

scheler commented 2 years ago

@dyladan @tigrannajaryan @jack-berg @martinkuba @MSNev I've made changes to follow the OTEP template and also removed the spec aspects. The draft spec content is at https://github.com/scheler/opentelemetry-specification/blob/events-and-logs-api/specification/logs/events-and-logs-api.md and I will submit separate PR later.

dyladan commented 2 years ago

Possibly it is covered somewhere in the conversation but why do we use the term "domain" for event instead of the term "scope" used by other signals?

tigrannajaryan commented 2 years ago

Possibly it is covered somewhere in the conversation but why do we use the term "domain" for event instead of the term "scope" used by other signals?

Scope is a runtime concept. It is defined as a place in the code:

A logical unit of the application code with which the emitted telemetry can be associated. It is typically the developer's choice to decide what denotes a reasonable instrumentation scope. The most common approach is to use the instrumentation library as the scope, however other scopes are also common, e.g. a module, a package, or a class can be chosen as the instrumentation scope.

Domain is not yet defined in the spec, we need a proper definition for it. The purpose of the domain is to group event definitions in semantic conventions such that event names only within the domain are unique, but between the domains the event names may overlap. The domain is a mechanism for namespacing event names. A domain can be allocated to a SIG and they are free to decide event names within the domain, knowing that any event names they choose are not going to be ambiguous and conflict with the event names in other domains. An event's semantic convention is essentially identified by a (domain.name, event.name) tuple.

dyladan commented 2 years ago

Possibly it is covered somewhere in the conversation but why do we use the term "domain" for event instead of the term "scope" used by other signals?

Scope is a runtime concept. It is defined as a place in the code:

A logical unit of the application code with which the emitted telemetry can be associated. It is typically the developer's choice to decide what denotes a reasonable instrumentation scope. The most common approach is to use the instrumentation library as the scope, however other scopes are also common, e.g. a module, a package, or a class can be chosen as the instrumentation scope.

Domain is not yet defined in the spec, we need a proper definition for it. The purpose of the domain is to group event definitions in semantic conventions such that event names only within the domain are unique, but between the domains the event names may overlap. The domain is a mechanism for namespacing event names. A domain can be allocated to a SIG and they are free to decide event names within the domain, knowing that any event names they choose are not going to be ambiguous and conflict with the event names in other domains. An event's semantic convention is essentially identified by a (domain.name, event.name) tuple.

Thanks for the description. I didn't understand that from the existing definition of domain in the OTEP so it might be helpful to add some details there. Beyond that this OTEP looks good to me.

tigrannajaryan commented 2 years ago

@open-telemetry/specs-logs-approvers please review.

tigrannajaryan commented 2 years ago

@open-telemetry/specs-approvers please review.

dyladan commented 2 years ago

The OTEP no longer contains the details that describe the API precisely and only contains the general description of the intent to add one. I think this is fine, we do allow OTEPs which are an "intent", with a purpose to seek alignment in Otel community. I agree with the intent and that's why I am approving the OTEP. The details of the Logs and Events API can be either a follow-up OTEP or direct PRs to the spec repo.

I think we should clarify what is meant to be in an OTEP and how far it is acceptable to diverge from the OTEP when writing spec. The specification parts of this OTEP were removed because of concerns that it was too prescriptive and didn't leave room for the spec to make any decisions if something came up in a language SIG which required a change. If we're just going to add those things back in a follow-up OTEP, then maybe they should be added back to this one (with my apologies to @scheler and others who removed those at least partially due to my comments)?

tigrannajaryan commented 2 years ago

I think we should clarify what is meant to be in an OTEP and how far it is acceptable to diverge from the OTEP when writing spec.

Normally some refinement happens when writing the spec. This is expected.

The specification parts of this OTEP were removed because of concerns that it was too prescriptive and didn't leave room for the spec to make any decisions if something came up in a language SIG which required a change. If we're just going to add those things back in a follow-up OTEP, then maybe they should be added back to this one (with my apologies to @scheler and others who removed those at least partially due to my comments)?

I think this OTEP in its current form is essentially an alignment document, which seeks agreement to add the Events and Logs API. It does not define the API itself and that's fine. We can define the API by working directly on the spec repo (like we did in the past for Traces).

tigrannajaryan commented 2 years ago

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers @open-telemetry/specs-logs-approvers @open-telemetry/specs-metrics-approvers We have enough approvals to merge. I am going to let this open for a couple more days, but unless I see objections I am going to merge this OTEP and proceed to defining/adding the new Events and Logs API to Otel specification.

tigrannajaryan commented 2 years ago

No new comments, no objections for the last 5 days. Merging.