Handle nulls in noop / multi #2939
Conversation
anuraaga
requested review from
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
Oberon00,
pavolloffay,
thisthat and
tylerbenson
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2939 +/- ##
=========================================
Coverage 91.04% 91.05%
- Complexity 2990 2995 +5
=========================================
Files 336 336
Lines 9316 9324 +8
Branches 946 949 +3
=========================================
+ Hits 8482 8490 +8
Misses 543 543
Partials 291 291
Continue to review full report at Codecov.
|
| @@ -45,13 +45,22 @@ | |||
|
|
|||
| @Override | |||
| public <C> void inject(Context context, @Nullable C carrier, TextMapSetter<C> setter) { | |||
| if (context == null || setter == null) { | |||
There was a problem hiding this comment.
I think letting the null bubble through to the inner propagators would be better, also for extract.
There was a problem hiding this comment.
Mostly just a gut feeling, and principle of least surprise. Also the Javadoc in https://github.com/open-telemetry/opentelemetry-java/blob/v0.17.1/api/context/src/main/java/io/opentelemetry/context/propagation/TextMapPropagator.java#L46-L53 TextMapPropagator.composite() states that it "simply delegates injection and extraction", so that's what I would do.
There was a problem hiding this comment.
Javadoc can certainly be updated. My opinion is that this is the safest approach, so that propagators that are not maintained by us will also be guarded against these bad inputs if they are wrapped up in a composite propagator with other "official" propagators.
There was a problem hiding this comment.
👍 I think that makes sense. If a user wants the other behavior, they can implement their own "NullNaiveMultiPropagator".
| } | ||
| if (getter == null) { | ||
| return context; | ||
| } | ||
| for (TextMapPropagator textPropagator : textPropagators) { | ||
| context = textPropagator.extract(context, carrier, getter); | ||
| } |
There was a problem hiding this comment.
As said above, I would not guard against null before this loop also here in extract, but only ensure that we don't return null even if null was passed in as context (and textPropagators was possibly empty).
There was a problem hiding this comment.
Per offline discussion with @jkwatson block this until the release
|
Let's continue the discussion after we release 1.0.0. Whatever we decide can go into the next minor release. |
Can we unblock this now? |
How about now, @bogdandrutu ? |
jkwatson
dismissed
bogdandrutu’s stale review
we're now post 1.0.0, so I think we can move forward with the discussion
|
@anuraaga looks like this needs a rebase. |
Fixes #2921
This may not be a good idea for Multi but went with consistency for now, let me know thoughts.