open-telemetry / opentelemetry-js

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

Disallow `new Span()` #3597

Open dyladan opened 1 year ago

dyladan commented 1 year ago

Currently it is possible to call new Span(). We should make the constructor private to disallow this.

Current test usages:

Possible additional goals:

JamieDanielson commented 3 months ago

Note: To do this, we can make Span private and for TypeScript we should be able to access with obj[“somePrivateProperty”]

pichlermarc commented 1 month ago

We may also be able to convert uses of Span to ReadableSpan wherever interfaces expect a ReadableSpan instead of an actual SDK span.

david-luna commented 1 month ago

Couple of questions:

pichlermarc commented 1 month ago
  • it should be no problem to leave usage of new Span in sdk-trace-base tests right?
  • should sdk-trace-base export the Span interface from @opentelemetry/api rather than the Span class?

Hmm, though questions.

I think the underlying goal should maybe rather be: don't export the Span class from @opentelemetry/sdk-trace-base at all, instead export an interface SdkSpan extends ReadableSpan, APISpan (this may need changes to ReadableSpan to align the spanContext property).

There's nothing wrong with keeping the Span type for internal use in that package, but we should reference the interface wherever possible, not the concrete class.

When going this route, the answer to the questions would be:

it should be no problem to leave usage of new Span in sdk-trace-base tests right?

yes, leaving it is no problem

should sdk-trace-base export the Span interface from @opentelemetry/api rather than the Span class?

Yes, wherever this fulfills the goals of the interface.

An API span is not readable. That's so that users don't use attributes they added to a span in their business logic to convey information, which makes a registered Otel SDK required for their app to function, but business logic should not be impacted, whether you have an OTel SDK registered or not.

In some SDK interfaces, the Span is readable on purpose though. SpanProcessor is an example - you need to be able to read the span to process it. We should provide access to the minimum interface to get the job done :slightly_smiling_face: