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

Lazy vs non-Lazy Event APIs #57

Closed
jmacd opened this issue Jul 16, 2019 · 4 comments
Closed

Lazy vs non-Lazy Event APIs #57

jmacd opened this issue Jul 16, 2019 · 4 comments
Labels
good first issue Good for newcomers pkg:API Related to an API package

Comments

@jmacd
Copy link
Contributor

jmacd commented Jul 16, 2019

The API currently has two forms of event API, one supporting non-lazy usage (Event) and one supporting lazy usage (AddEvent). On the one hand, there is a desire to support lazy events. On the other hand, there is a desire not to force allocation of an object for all events.

This may rise to a specification level issue. We have open discussions around SpanData and Sampling and Laziness in general, and it is difficult to separate the event API from this larger discussion.

In OpenTracing, there was support for lazy events, via bulk delivery using an option to the span.Finish() call. I believe the thinking was that sometimes you want to be lazy and limit the number of events that are buffered (assuming they are buffered). See https://github.com/opentracing/opentracing-go/blob/135aa78c6f95b4a199daf2f0470d231136cbbd0c/span.go#L154

At the same time, some OpenTracing libraries added support for lazy values, which are not the same as lazy events. In this case, I believe the thinking was that a programmer is only concerned with avoiding an expensive serialization when the SDK is not recording a span. See https://github.com/opentracing/opentracing-go/blob/135aa78c6f95b4a199daf2f0470d231136cbbd0c/log/field.go#L149

I will propose that we:

  1. Eliminate the lazy/interface form "AddEvent", keep the non-lazy/concrete form "Event". Either name will do, it's the behavior that counts.
  2. Support bulk lazy events via FinishOptions as in OpenTracing
  3. Support lazy values via a new ValueType for lazy serialization
@bogdandrutu
Copy link
Member

Lazy events at the end is not necessarily good. For example you force everyone to make these data available in the function that calls end. Also some implementation may want to allow access in real time to these process (what OpenCensus calls zpages active spans).

Another usecase is that you the user to also record a timestamp for these events because they don't happen at the end of the Span.

@bogdandrutu
Copy link
Member

My 2 cents here is to eliminate the Event interface and use a concrete type + formatter to avoid allocation when possible and keep that API

@jmacd
Copy link
Contributor Author

jmacd commented Jul 16, 2019

In the OpenTelemetry FinishOptions, each log record has an independent timestamp.

I also support the idea that Event (or AddEvent) should use a concrete type + formatter, it's exactly what I mean when I say "lazy value". Sounds like this will be easy to resolve.

@rghetia rghetia added the pkg:API Related to an API package label Jul 16, 2019
@jmacd
Copy link
Contributor Author

jmacd commented Aug 16, 2019

Today's API reads:

	// AddEvent adds an event to the span.
	AddEvent(ctx context.Context, event event.Event)
	// AddEvent records an event to the span.
	Event(ctx context.Context, msg string, attrs ...core.KeyValue)

I believe we should eliminate one of these. The specification says the method should be named "AddEvent", but we have agreed above in this issue not to use an interface.

Therefore, we should consolidate into one method

   AddEvent(ctx context.Context, msg string, attrs ...core.KeyValue)

and remove the Event interface.

@jmacd jmacd added the good first issue Good for newcomers label Aug 16, 2019
krnowak added a commit to kinvolk/opentelemetry-go that referenced this issue Aug 16, 2019
There was an agreement to get rid of the Event interface and
consolidating the two methods for adding events into one. See open-telemetry#57.
krnowak added a commit to kinvolk/opentelemetry-go that referenced this issue Aug 27, 2019
There was an agreement to get rid of the Event interface and
consolidate the two methods for adding events into one. See open-telemetry#57.
rghetia pushed a commit that referenced this issue Sep 3, 2019
* Merge two event methods in span API

There was an agreement to get rid of the Event interface and
consolidate the two methods for adding events into one. See #57.

* Eliminate the use of the Event interface

There is no need for the SDK to provide the implementation of the
Event interface - it is used nowhere.

* Drop the Event interface

It's dead code now.

* Make it possible to override a finish timestamp through options

Opentracing to opentelemetry bridge will certainly use this feature.

* Obey the start time option

* Add tests for events and custom start/end times
@jmacd jmacd closed this as completed Sep 5, 2019
hstan referenced this issue in hstan/opentelemetry-go Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers pkg:API Related to an API package
Projects
None yet
Development

No branches or pull requests

3 participants