search

open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.73k stars 795 forks source link

[Discussion] Deprecating the addNetworkSpanEvents as this is not the correct approach for "events" #3174

Open MSNev opened 2 years ago

MSNev commented 2 years ago

Looking through the code the helper functions (which appear to only be getting used by the experiential instrumentation) to "add" resource details as individual events on a span is the wrong approach.

Looking over the history it appears that these were added to support the experimental http/fetch instrumentations only and at the time this was the only way to hack the details to send them off.

https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-web/src/utils.ts#L51-L105

I'm not aware of any semantic conventions around these and therefore they should at the very least be experimental functions and the way the client RUM events are being defined these individual events will be problematic as well as an issue for backend to "stitch" together individual events again.

There are also limits imposed on the number of events that can be included in a span (although current usage of these specific helper functions would not hit these scenarios).

While I don't discount the need for the helpers, the way they "add" the details is incorrect and I don't believe conforms to the general rational for spans having events

As the experimental XHR and fetch instrumentations are using these functions I would also be STRONGLY opposed to them becoming stable while using these functions in this form.

MSNev commented 2 years ago

The "more" correct approach that the addSpanNetworkEvents SHOULD be doing is creating a SINGLE event and having the available values as Attributes.

t2t2 commented 2 years ago

I don't believe conforms to the general rational for spans having events

Isn't it a bit premature to claim spans with events to hold timing information over SpanEvents when there's 0 specification changes or OTEPs actually saying that yes it should be done this way?

vmarchaud commented 2 years ago

I agree that we should not decaclare them stable (althrough i don't think there is plan to declare any instrumentations stable yet ?), specially with the RUM SIG that will most likely specificied a bunch of things that will change data collections (but you know more than me on this part). I do wonder if that's worth to change right now before more standardization ?

scheler commented 2 years ago

The "more" correct approach that the addSpanNetworkEvents SHOULD be doing is creating a SINGLE event and having the available values as Attributes.

I support this proposal. The events can use PerformanceEntry.entryType as their name.

The current array structure (of events) makes it hard to look entries up by name on the backend and requires converting to a map.

scheler commented 2 years ago

Isn't it a bit premature to claim spans with events to hold timing information over SpanEvents when there's 0 specification changes or OTEPs actually saying that yes it should be done this way?

Just to be clear, the ask here is to use a SINGLE Span.Event instead of multiple, and not a standalone Event.

t2t2 commented 2 years ago

Just to be clear, the ask here is to use a SINGLE Span.Event instead of multiple, and not a standalone Event.

Ah yeah, using span.event attributes does have a slight issue that zipkin exporter (which is currently more popular for exporting data from web apps) isn't aware of translating otel event attributes:

https://github.com/open-telemetry/opentelemetry-js/blob/34c5bdba67f2029c4fb046307d064c2e96ae8b5a/packages/opentelemetry-exporter-zipkin/src/transform.ts#L98

Spec does include a suggested conversion format for it

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] commented 1 year ago

This issue was closed because it has been stale for 14 days with no activity.

dyladan commented 1 year ago

To my knowledge this isn't widely used outside of our own instrumentations. It may be possible to at least deprecate these before 2.0 and just wait for 2.0 to do the actual removal.

Possible deprecation strategy: