Allow specifying a timestamp when creating Events

[johananl]

The spec defines an Event as follows:

A set of zero or more Events, each of which is itself a key:value map paired with a timestamp.

The way I understand it, it doesn't make sense to have an Event without a timestamp.

Right now the API has two methods for adding Events: add_event() which constructs an event and appends it to the Span, and add_lazy_event() which accepts a pre-created Event object and appends it to the Span.

opentelemetry-python/opentelemetry-api/src/opentelemetry/trace/__init__.py

Lines 180 to 187 in b0da53d

	def add_event(
	self, name: str, attributes: types.Attributes = None
	) -> None:
	"""Adds an `Event`.
	Adds a single `Event` with the name and, optionally, attributes passed
	as arguments.
	"""

opentelemetry-python/opentelemetry-api/src/opentelemetry/trace/__init__.py

Lines 189 to 193 in b0da53d

	def add_lazy_event(self, event: Event) -> None:
	"""Adds an `Event`.
	Adds an `Event` that has previously been created.
	"""

At the moment, the only way to explicitly specify a timestamp is as follows:

1. Create an Event manually by instantiating Event().
2. Call add_lazy_event() while passing the Event created previously.

This isn't very flexible since in some cases we may want to create an event while letting the OTel implementation generate a timestamp for us, whereas in other cases we may want to explicitly specify a timestamp. Right now the only way for us to address both needs is to conditionally call either add_event() or add_lazy_event(), which is confusing in my opinion.

The above creates a problem for the OT bridge for example: on one hand we want to have an optional timestamp argument when logging events. On the other hand, when a timestamp isn't specified, our only option right now is to generate a timestamp in the bridge, that is - outside of OTel, and pass a constructed object to add_lazy_event().

I propose to do the following:

• Add an optional timestamp argument to add_event on the API with a default of None.
• State in the API documentation that implementations should generate a timestamp if timestamp is None, and let the implementations decide what kind of timestamp to use and how to generate it.
• Make add_event() in the SDK generate a timestamp only if timetstamp is None (right now it always generates a timestamp).

The changes could look as follows:

diff --git opentelemetry-api/src/opentelemetry/trace/__init__.py opentelemetry-api/src/opentelemetry/trace/__init__.py
index 18eced4..5cfed38 100644
--- opentelemetry-api/src/opentelemetry/trace/__init__.py
+++ opentelemetry-api/src/opentelemetry/trace/__init__.py
@@ -178,12 +178,16 @@ class Span:
         """
 
     def add_event(
-        self, name: str, attributes: types.Attributes = None
+        self,
+        name: str,
+        timestamp: int = None,
+        attributes: types.Attributes = None,
     ) -> None:
         """Adds an `Event`.
 
-        Adds a single `Event` with the name and, optionally, attributes passed
-        as arguments.
+        Adds a single `Event` with the name and, optionally, a timestamp and
+        attributes passed as arguments. Implementations should generate a
+        timestamp if the `timestamp` argument is omitted.
         """
 
     def add_lazy_event(self, event: Event) -> None:

diff --git opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
index eb754fa..b99b372 100644
--- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
+++ opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
@@ -198,12 +198,15 @@ class Span(trace_api.Span):
         self.attributes[key] = value
 
     def add_event(
-        self, name: str, attributes: types.Attributes = None
+        self,
+        name: str,
+        timestamp: int = None,
+        attributes: types.Attributes = None,
     ) -> None:
         self.add_lazy_event(
             trace_api.Event(
                 name,
-                time_ns(),
+                time_ns() if timestamp is None else timestamp,
                 Span.empty_attributes if attributes is None else attributes,
             )
         )

[johananl]

I've included a proposed implementation as part of #211.

[Oberon00]

@johananl I guess this is done now? Can it be closed?

[johananl]

Yes, and sorry for the slow response.