feat: never return undefined when the method expects an object

[dmathieu]

I believe returning undefined when the context has no span is invalid per the specification:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/error-handling.md#guidance

Whenever API call returns values that is expected to be non-null value - in case of error in processing logic - SDK MUST return a "no-op" or any other "default" object that was (ideally) pre-allocated and readily available. This way API call sites will not crash on attempts to access methods and properties of a null objects.

See also the discussion in https://github.com/open-telemetry/oteps/issues/216

Note: this may be deemed a breaking change, in which case I am happy to rename the commit to feat, and to wait for the next planned appropriate release.

[codecov]

Codecov Report

Base: 95.23% // Head: 95.39% // Increases project coverage by +0.16% 🎉

Coverage data is based on head (84555ff) compared to base (d2fb706).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     open-telemetry/oteps#175      +/-   ##
==========================================
+ Coverage   95.23%   95.39%   +0.16%     
==========================================
  Files          45       45              
  Lines         650      651       +1     
  Branches      102      101       -1     
==========================================
+ Hits          619      621       +2     
+ Misses         31       30       -1     

Impacted Files	Coverage Δ
src/baggage/context-helpers.ts	88.88% <100.00%> (+1.38%)	⬆️
src/baggage/internal/baggage-impl.ts	100.00% <100.00%> (+3.84%)	⬆️
src/trace/context-utils.ts	94.11% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Flarna]

marking with request change until the spec part is clear regarding this.

If we go this root we should not stick on getSpan instead we should fix this for all context methods (e.g. baggage,...).

[dmathieu]

I have added this to every other public method which returned undefined (and not a string). This is way farther from my area of confidence in the JS API though. So I may have messed up.

Not having this is currently forcing us into writing a lot of defensive conditions. Basically, whenever we retrieve the current span from context, we need to check if there is indeed a span, which is quite verbose.

[legendecas]

This removes the ability to determine if a baggage entry is present in the baggage. I'd find this line of change unexpected regarding the OP:

Whenever API call returns values that is expected to be non-null value - in case of error in processing logic

The undefined returned here is not an error.

[dyladan]

I don't think returning undefined is invalid.

Whenever API call returns values that is expected to be non-null value - in case of error in processing logic - SDK MUST return a "no-op" or any other "default" object that was (ideally) pre-allocated and readily available. This way API call sites will not crash on attempts to access methods and properties of a null objects.

IMO, this method is not expected to return a non-null value. The docs explicitly state that it may be undefined. Further, this spec guidance is specifically around error handling and what you've changed is the normal operation of the API. This change could also be interpreted as breaking.

This package is being moved to the main https://github.com/open-telemetry/opentelemetry-js repository and I would suggest that if you feel this is an important issue you open an issue there.

[dyladan]

I went ahead and created a discussion issue over there open-telemetry/opentelemetry-js#3316