[ISSUE-5161] Fix panics if a `nil` context is provided to the `.Start()` method

[htemelski]

Fix for https://github.com/open-telemetry/opentelemetry-go/issues/5161

As agreed by this discussion, the implementation should not throw unhandled exceptions at run time.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: xxbtwxx / name: Hristo Temelski (eb1a0115330965f6f1c7c9e44a795f2043db298d, affde0d66ac4df34339206be4459f68dff7daa2d, 0f99c8a5c76754a601702402e2d15a4fb2abaa33, 7298e2716e3f3f27b93a828c2806677c922b1a45, 4351cb31c66989fc45a619da686e700983076b57, c7f4d786b207064b133b7ebf63209b7d7f8f9608)

[dmathieu]

Could you add tests?

[MrAlias]

This seems like standard Go behavior. The context package explicitly states nil context should not be used:

I do not think we should be adding boilerplate to all of our functions to prevent invalid user behavior in the eyes of Go.

[htemelski]

This seems like standard Go behavior. The context package explicitly states nil context should not be used:

I do not think we should be adding boilerplate to all of our functions to prevent invalid user behavior in the eyes of Go.

This was previously discussed and agreed upon here

Here's a link to the spec, that says:

API methods MUST NOT throw unhandled exceptions when used incorrectly by end users. The API and SDK SHOULD provide safe defaults for missing or invalid arguments. For instance, a name like empty may be used if the user passes in null as the span name argument during Span construction.

[dmathieu]

I may be doing lawyer talk, but I'm not sure the specification works for this case.

API methods MUST NOT throw unhandled exceptions when used incorrectly by end users.

I read this specification as saying that the SDK's API methods should not throw unhandled exceptions when the SDK's API methods are used incorrectly by end-users. Not if an external API (in this case, context.Context) is used incorrectly.

This was previously discussed and agreed upon https://github.com/open-telemetry/opentelemetry-go/pull/3110#issuecomment-1227253604

This comment is not an agreement. It's a single approver's opinion (which is not to be dismissed either of course). cc @dashpole

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.8%. Comparing base (014c6fc) to head (c7f4d78). Report is 427 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5162   +/-   ##
=====================================
  Coverage   83.8%   83.8%           
=====================================
  Files        254     254           
  Lines      16532   16538    +6     
=====================================
+ Hits       13864   13870    +6     
  Misses      2375    2375           
  Partials     293     293           

Files with missing lines	Coverage Δ
internal/global/trace.go	85.1% <100.0%> (+0.6%)	:arrow_up:
trace/noop.go	59.0% <100.0%> (+4.0%)	:arrow_up:
trace/noop/noop.go	100.0% <100.0%> (ø)

[htemelski]

I may be doing lawyer talk, but I'm not sure the specification works for this case.

API methods MUST NOT throw unhandled exceptions when used incorrectly by end users.

I read this specification as saying that the SDK's API methods should not throw unhandled exceptions when the SDK's API methods are used incorrectly by end-users. Not if an external API (in this case, context.Context) is used incorrectly.

I would agree with this if the second part of the spec was not included The API and SDK SHOULD provide safe defaults for missing or invalid arguments. With a nil context clearly being an invalid argument

[dashpole]

My reading of the spec is that we are required not to panic on nil context for API methods. It is also an improper use of go's Context, but i'm not sure that changes anything.

This comment is not an agreement. It's a single approver's opinion

Thats correct. I'm happy to be overruled by other approvers here.

But I don't think this needs to be a lot of boilerplate, since we don't actually need to add a nil check everywhere. IMO we should just update ContextWithSpan to handle a nil parent context here:

https://github.com/open-telemetry/opentelemetry-go/blob/014c6fc3323554065532b2b260945a9d0cd62b74/trace/context.go#L13-L15

I've checked all of our metric and trace APIs that accept a context.Context, and all of our usage is either to call trace.SpanFromContext, or trace.ContextWithSpan (or a method that calls one of those two). We already check for nil in trace.SpanFromContext: https://github.com/open-telemetry/opentelemetry-go/blob/014c6fc3323554065532b2b260945a9d0cd62b74/trace/context.go#L37-L40

If we did decide to go that route, it would be good, IMO to add the TestNoPanic tests from this PR, and also to our metrics and logs APIs.

[pellared]

since we don't actually need to add a nil check everywhere

What about all API methods that accepts interfaces like Span, SpanStartOption, TracerOption, Int64CounterOption?

E.g. do we want to add nil-checks in the code below?

https://github.com/open-telemetry/opentelemetry-go/blob/66284071defe2c6b2261aecfa506f37d6e2a31f2/metric/asyncint64.go#L108-L114