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

Consider renaming "EventFormatter" #533

Closed
toumorokoshi opened this issue Mar 29, 2020 · 6 comments · Fixed by #840
Closed

Consider renaming "EventFormatter" #533

toumorokoshi opened this issue Mar 29, 2020 · 6 comments · Fixed by #840
Labels
priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory

Comments

@toumorokoshi
Copy link
Member

For lazy events, the current API requires the attribute creation function to be called "EventFormatter":

An API to record a single Event whose attributes or attribute values are
lazily constructed, with the intention of avoiding unnecessary work if an event
is unused. If the language supports overloads then this SHOULD be called
AddEvent otherwise AddLazyEvent MAY be considered. In some languages, it
might be easier to defer Event or attribute creation entirely by providing a
wrapping class or function that returns an Event or formatted attributes. When
providing a wrapping class or function it SHOULD be named EventFormatter

I feel that the name is a little bit of a misnomer, as there are multiple reasons why event attribute population would be better of lazy (e.g. having to perform some synchronous request to retrieve said values), and formatting may just be one.

As an alternative, I might call it "EventRetriever".

@Oberon00
Copy link
Member

I wonder if we should just remove lazy Events from the spec. Since the SDK only supports head-based sampling anyway, a check of the IsRecording property on the caller side should be even more effective in avoiding work.

@toumorokoshi
Copy link
Member Author

I think the use cases for such a class are fairly limited, and I have a feeling formatting will not be the reason why you want deferred attributes.

But on the other hand, if it is a common enough need, we should have it specified somewhere.

As a slightly more complex alternative, one could create a lazy API, and use that for the non-lazy case as well, since that would just be returning back an already-constructed object.

@bogdandrutu
Copy link
Member

@Oberon00 lazy Events can be used even with head-based sampling. You can postpone the formatting until the exporter phase.

@Oberon00
Copy link
Member

Oberon00 commented Apr 1, 2020

This only makes a difference vs checking IsRecording in edge cases tough, like if the BatchSpanProcessor queue is full (and even then only if the implementation does not call toSpanData or equivalent right at the Span.end call).

@bogdandrutu bogdandrutu added the spec:trace Related to the specification/trace directory label Jun 12, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 10, 2020
@andrewhsu andrewhsu added the priority:p2 Medium priority level label Jul 17, 2020
@bogdandrutu
Copy link
Member

@Oberon00 I think IsRecording should be good enough. Can you create a PR to remove the lazy or make it optional?

@arminru
Copy link
Member

arminru commented Aug 20, 2020

@bogdandrutu Do you know if any of the language implementation actually implement lazy event creation/EventFormatter? Java, for example, does not and thus actually breaches this MUST requirement.

I'd be in favor of removing the lazy event API entirely as well as the lazy link API. I'm not aware of any reason for the inconsistency between links/events attributes and span attributes as there's no lazy SetAttribute API and such either. It seems checking IsRecording should be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants