open-telemetry / opentelemetry-js

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

OpenTracing Shim does not respect span.kind tag #3797

Open mohitk05 opened 1 year ago

mohitk05 commented 1 year ago

What happened?

The OpenTracing shim does not correctly reflect the span kind as tagged by the host application traced with OpenTracing. Currently all spans exported using the shim get the kind property in the span object as 0 coming from SpanKind.Internal.

Steps to Reproduce

Hello! The following steps are performed in order to have OTLP data exported from a service traced with OpenTracing.

  1. Initialize the tracer using the OpenTelemetry Node.js packages.
  2. Pass the tracer to the TracerShim to obtain an OpenTracing compatible tracer.
  3. Initialize this tracer globally in OpenTracing using opentracing.initGlobalTracer.
  4. Add the span.kind tag to a span.

Expected Result

The exported OTLP spans should have the correct kind property, i.e. a corresponding value as specified by the host application while creating the span, mapped to the OTel SpanKind.

Actual Result

All spans generated by the shim have kind internal.

Additional Details

This seems to be happening because the shim does not pass a kind property in options while creating a new Span using tracer.startSpan. This method internally assigns kind internal if no kind was passed in options. Source:

  1. https://github.com/open-telemetry/opentelemetry-js/blob/8fc76896595aac912bf9e15d4f19c167317844c8/packages/opentelemetry-shim-opentracing/src/shim.ts#L139
  2. https://github.com/open-telemetry/opentelemetry-js/blob/8fc76896595aac912bf9e15d4f19c167317844c8/packages/opentelemetry-shim-opentracing/src/shim.ts#L41
  3. https://github.com/open-telemetry/opentelemetry-js/blob/8fc76896595aac912bf9e15d4f19c167317844c8/packages/opentelemetry-sdk-trace-base/src/Tracer.ts#L102

I checked the specification for compatibility and could not find any detail about span.kind. What would be the approach here? Without this property, it is difficult to start using the shim in services that need this data (and already export this property) to port quickly to OpenTelemetry. Your suggestions would be helpful!

OpenTelemetry Setup Code

// We have a small wrapper around OTel packages. Finally, a tracer is obtained from BasicTracerProvider

import api from '@opentelemetry/api';
import { BasicTracerProvider } from '@opentelemetry/sdk-trace-base';
import { TracerShim } from '@opentelemetry/shim-opentracing';

...

const tracer = api.trace.getTracer('default');
opentracing.initGlobalTracer(new TracerShim(tracer));
...
// Creating a span with OpenTracing
const span = tracer.startSpan('span', { /* options */ });
span.addTags({
  [Tags.SPAN_KIND]: Tags.SPAN_KIND_RPC_SERVER // 'span.kind': 'server'
})

package.json

No response

Relevant log output

// Exported span looks like
{
  "name": "span",
  ...
  "kind": 0 // 0 -> SpanKind.Internal
}
Flarna commented 1 year ago

Hmm, that seems to be not that easy. OTel expects the kind on span start but above code indicates that OpenTracing allows to set it later.

As far as I have the history of this in mind OTLP was defined in context of OpenTelemetry, not OpenTracing. For OpenTracing the kind was more an attribute. There are more differences like instrumentation scope which more or less identifies the instrumenation library and is set implicit at span start which was just some attribute in OT.

I fear a 100% mapping of older opentracing to the newer opentelemetry features is maybe not possible.

mohitk05 commented 1 year ago

@Flarna I assumed this will be difficult, can the shim though try to assign the kind if the OpenTracing method startSpan receives the span.kind tag in options when the span is created? Also the types of values for kind look similar across OTel and OpenTracing: OTel: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#spankind OpenTracing: https://github.com/opentracing/specification/blob/master/semantic_conventions.md#span-tags-table

Assuming these can be related to each other, the shim may try to do something like:

function translateSpanOptions(
  options: opentracing.SpanOptions
): api.SpanOptions {
  const opts: api.SpanOptions = {
    startTime: options.startTime,
    kind: mapKind(options.tags?[opentracing.Tags[SPAN_KIND]])
  };

  if (options.references) {
    opts.links = translateReferences(options.references);
  }

  return opts;
}

If the span.kind property is not sent as a tag while span creation, the span gets the kind internal, like the current behaviour. What do you think?

Flarna commented 1 year ago

I think remapping if Tags.SPAN_KIND is set on start would work. But I'm not sure if the user experience is good if kind is "correct" only sometimes. OpenTracing is archived since quite a while therefore I think it's unlikely that existing code base will be adapted to fit OtelShim. All these locations should be moved to use Otel directly.

There are other OT tags which have a special meaning now, e.g. component would be now the instrumentation scope. OTel has also a bunch of semantic conventions for http,... and I doubt we should try to convert them.

mohitk05 commented 1 year ago

Okay, understood, and I agree with you.

If this cannot be done at the package level, can the shim export all the 3 named exports? Applications can then willingly implement this logic on their end if necessary.

https://github.com/open-telemetry/opentelemetry-js/blob/8fc76896595aac912bf9e15d4f19c167317844c8/packages/opentelemetry-shim-opentracing/src/index.ts#L17

Currently only the TracerShim is exported, for overriding startSpan one would also need SpanShim and SpanShimContext. I can raise a PR for this if you think this is okay.

Flarna commented 1 year ago

Not sure what spec allows here. And I wonder if this really helps because the underlying span doesn't allow to set kind after start.

@open-telemetry/javascript-maintainers Do you know the details what OT shim should/is allowed to expose? Or any other ideas how to better transform OT to OTel?

mohitk05 commented 1 year ago

Hello! I think the tag did not work in the previous comment.

And I wonder if this really helps because the underlying span doesn't allow to set kind after start.

I was planning to override it to accept the kind as a tag during span creation. This way the kind property works with OTel and I can make the change in the host application.

aabmass commented 1 year ago

It's worth calling out in the README, or possibly the specification (there are some similar caveats in the OpenCensus compatibility spec for example). You could also add a debug log that span.kind attribute is not being mapped to span kind so at least there is some observability.

pichlermarc commented 1 year ago

According to the specification this behavior is correct, I think this is something the need to handle on the spec first.

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.