chore: remove SpanOptions.isRecording

[Flarna]

Which problem is this PR solving?

Fixes: #811

Short description of the changes

Remove SpanOptions.isRecording as it seems to be quite usless and I found
only Span.isRecording in spec.

Also Java seems to have no such option for span creation.

[codecov-io]

Codecov Report

Merging #817 into master will decrease coverage by 30.61%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #817       +/-   ##
===========================================
- Coverage   92.63%   62.02%   -30.62%     
===========================================
  Files         244       71      -173     
  Lines       10774     1630     -9144     
  Branches     1064      235      -829     
===========================================
- Hits         9981     1011     -8970     
+ Misses        793      619      -174

Impacted Files	Coverage Δ
packages/opentelemetry-api/src/trace/NoopTracer.ts	100% <ø> (ø)	⬆️
...-api/test/noop-implementations/noop-tracer.test.ts	0% <ø> (-53.58%)	⬇️
...pentelemetry-core/src/platform/node/performance.ts	0% <0%> (-100%)	⬇️
...try-core/test/trace/fixtures/test-package/index.js	0% <0%> (-100%)	⬇️
...telemetry-core/test/platform/hex-to-base64.test.ts	0% <0%> (-100%)	⬇️
...st/trace/fixtures/test-package/foo/bar/internal.js	0% <0%> (-100%)	⬇️
...ntelemetry-core/src/platform/node/hex-to-base64.ts	0% <0%> (-88.89%)	⬇️
...ckages/opentelemetry-core/test/platform/id.test.ts	0% <0%> (-77.78%)	⬇️
...entelemetry-core/test/common/ConsoleLogger.test.ts	0% <0%> (-77.02%)	⬇️
...ntelemetry-core/test/trace/NoRecordingSpan.test.ts	0% <0%> (-71.43%)	⬇️
... and 185 more

[mayurkale22]

LGTM, however span.isRecording() is not getting used anywhere in the code. Same with Java's implementation. Any idea?

[dyladan]

@mayurkale22 not sure why Span.isRecording() exists honestly. I did some spelunking in the history and found that it was added in one of the very first spec PRs with the description "Continue moving documents to the root folder from OpenTracing and OpenCensus original documents," but I can't find its specification in either of those two projects. I think in order to find out why it exists you would have to dig into one of those projects more deeply.

[Flarna]

I have two thoughts regarding span.isRecording():

• Adapt trace.Span.isRecording() it to return false once span has ended
• use it in plugins to avoid extracting attributes for nothing

[dyladan]

The IsRecording section of the trace API gives a little more info, but not much https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#isrecording.

This flag may be true despite the entire trace being sampled out. This allows to record and process information about the individual Span without sending it to the backend. An example of this scenario may be recording and processing of all incoming requests for the processing and building of SLA/SLO latency charts while sending only a subset - sampled spans - to the backend.

This seems to imply to me that you may want to have a span that is sampled: false, but is not a noop span. The easiest way I can think of to do this is to have the noop span return false for isRecording, and have the "working" span return true for isRecording regardless of its traceflags.

[Flarna]

Default SDK decides on sampling durnig startSpan and I think it makes no sense to record data if span is not sampled.
But it would be possible to replace the SDK and do lazy sampling. Therefore start with a recording span but some time later (before exporting) it decides to not export it (e.g. bandwith, statistics, attributes added later,..).

[dyladan]

Default SDK decides on sampling durnig startSpan and I think it makes no sense to record data if span is not sampled.
But it would be possible to replace the SDK and do lazy sampling. Therefore start with a recording span but some time later (before exporting) it decides to not export it (e.g. bandwith, statistics, attributes added later,..).

I agree about lazy sampling, although it is of marginal value because you need to send some sampled flag with an outgoing request anyway.

The text I quoted from the spec clearly states that having a sampled=no recording=yes span is desirable. Whether it is a good reason is unclear to me, but someone obviously wanted it there.

[Flarna]

The sampling flag section in W3C trace context contains mainly SHOULD and MAY.

Whether it is a good reason is unclear to me, but someone obviously wanted it there.

As far as I remember quite some parts were copied from OpenTracing/OpenCensus and not everything has been 100% consolidated/repolished.

Most likely it would make sense to improve spec regarding this before moving to beta to ensure a consistent implementation across techs.

[dyladan]

After conversations with spec maintainers, it seems they want noop spans to return false for this, and "working" spans to return true regardless of if they are sampled. This allows for delayed sampling decisions and other statistics gathering beyond sending the spans to a tracing backend.

edit: to be clear, that is the behavior of Span.isRecording() and is already implemented correctly. This PR is removing unwanted options and is 👍 IMO.

[Flarna]

Seems HttpPlugin tests are flaky. Should I do a dummy commit to retrigger CI? Or how is this handled in general?

  1) HttpPlugin Integration tests
       enable()
         should create a rootSpan for GET requests and add propagation headers:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/root/project/packages/opentelemetry-plugin-http/test/integrations/http-enable.test.ts)
      at listOnTimeout (internal/timers.js:549:17)
      at processTimers (internal/timers.js:492:7)

[dyladan]

I just retriggered the failed build for you. I think we should take some time to clean up flaky tests. There is also a dns test that is an issue.

[mayurkale22]

I think this is good to merge, please resolve the conflicts.

[Flarna]

I think this is good to merge, please resolve the conflicts.

done

[OlivierAlbertini]

I just retriggered the failed build for you. I think we should take some time to clean up flaky tests. There is also a dns test that is an issue.

Do you mean that you want to remove integretion tests? Because it’s network issue.