opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

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

Open MSNev opened this issue 2 years ago • 6 comments

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.

  • [X] This only affects the JavaScript OpenTelemetry library
  • [ ] This may affect other libraries, but I would like to get opinions here first

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 avatar Aug 17 '22 17:08 MSNev

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

MSNev avatar Aug 17 '22 22:08 MSNev

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?

t2t2 avatar Aug 18 '22 15:08 t2t2

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 ?

vmarchaud avatar Aug 20 '22 07:08 vmarchaud

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 avatar Aug 20 '22 17:08 scheler

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.

scheler avatar Aug 20 '22 18:08 scheler

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

t2t2 avatar Aug 22 '22 08:08 t2t2

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] avatar Nov 07 '22 06:11 github-actions[bot]

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

github-actions[bot] avatar Nov 28 '22 06:11 github-actions[bot]

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:

  • Use @deprecated flag on the function
  • Add new "better" events to existing instrumentation
  • Add a (default false) configuration to the instrumentation to not export deprecated events (omitDeprecatedNetworkSpanEvents: true)
  • optional: add a (default true) configuration to include the new "better" event
  • In 2.0 remove both configurations and make their defaults the only behavior

dyladan avatar Feb 09 '23 13:02 dyladan