Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Consider hiding/moving Event/TimedEvent #37

Closed
carlosalberto opened this issue Apr 20, 2021 · 8 comments · Fixed by open-telemetry/opentelemetry-js#2183, open-telemetry/opentelemetry-js-contrib#470 or #60
Assignees

Comments

@carlosalberto
Copy link

carlosalberto commented Apr 20, 2021

Span.addEvent() does not use these types, so it may be worth hiding them (or exposing them in the SDK level, in case they are used around there).

Also, as all events are expected to have a timestamp, maybe a single class could have all of this.

@dyladan
Copy link
Member

dyladan commented Apr 20, 2021

Moving them to SDK probably makes sense. You are correct they are not used at all in the API.

@obecny
Copy link
Member

obecny commented Apr 22, 2021

PG Plugin and Document Load Plugin are using TimedEvent, and both of them don't depend on core at all. Moving those interfaces to core will require to depend on core. Because of that I think we should consider leaving the interfaces in api. We did a lot of work to remove dependency from core in instrumentation and I would like to avoid the dependency from core.

@open-telemetry/javascript-maintainers ?

@vmarchaud
Copy link
Member

Agree, we stated that we aim for the instrumentations to only depend on the API so they can be re-used with 3rd party SDK if needed

@dyladan
Copy link
Member

dyladan commented Apr 27, 2021

What are they using them for? There are no API methods that take them as arguments or return them as values.

@obecny
Copy link
Member

obecny commented Apr 28, 2021

TimedEvent is part of ReadableSpan, is also used in couple other places, TimedEvent contains also SpanAttributes and then SpanAttributeValue

@dyladan
Copy link
Member

dyladan commented Apr 29, 2021

ReadableSpan is also not part fo the API. An SDK type could contain API types so the fact it contains attributes isn't a problem.

@obecny
Copy link
Member

obecny commented May 3, 2021

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#add-events
So the api defines such event. Our sdk uses this interfaces too so I don't think we should be removing it, some other sdk / instrumentations / etc. might depend on this too.

@dyladan
Copy link
Member

dyladan commented May 3, 2021

The type isn't referenced anywhere though. It is just a top-level export that isn't referenced by any function arg or return. Our addEvent function takes a name, timestamp, and attributes. It doesn't take an Event type and it doesn't return an Event type.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.