Return no-op when segment context isn't found instead of null. #180
Conversation
| AWSXRayRecorder recorder = AWSXRayRecorderBuilder.standard() | ||
| .withContextMissingStrategy(new LogErrorContextMissingStrategy()) | ||
| .withContextMissingStrategy(new IgnoreErrorContextMissingStrategy()) |
There was a problem hiding this comment.
These tests don't actually test whether anything's logged so I just removed that to make them more precise.
There was a problem hiding this comment.
Looks great, big improvement on customer expectations & safety. My question is for getCurrentSegment(), will we leave that Nullable? Or do we want to change that long term to also return a DummySegment when context is missing, and it's just outside the scope of this PR?
| SegmentContext context = segmentContextResolverChain.resolve(); | ||
| if (context != null) { | ||
| return context.beginSubsegment(this, name); | ||
| if (context == null) { | ||
| // No context available, we return a no-op subsegment so user code does not have to work around this. Based on | ||
| // ContextMissingStrategy they will still know about the issue unless they explicitly opt-ed out. |
There was a problem hiding this comment.
I'm not actually sure the contextMissingStrategy would get fired in this case. I think this is a bug in the old implementation, and we should be using getSegmentContext() instead of resolving the it from the ResolverChain.
| @@ -352,7 +345,7 @@ public Segment preFilter(ServletRequest request, ServletResponse response) { | |||
| final Segment created; | |||
| if (SampleDecision.SAMPLED.equals(sampleDecision)) { | |||
| created = recorder.beginSegment(getSegmentName(httpServletRequest), traceId, parentId); | |||
| if (created != null && samplingResponse.getRuleName().isPresent()) { | |||
| if (samplingResponse.getRuleName().isPresent()) { | |||
| logger.debug("Sampling strategy decided to use rule named: " + samplingResponse.getRuleName().get() + "."); | |||
| created.setRuleName(samplingResponse.getRuleName().get()); | |||
| } | |||
There was a problem hiding this comment.
Think you can get rid of the other null check in the else block
There was a problem hiding this comment.
Good call on getCurrentSegment - I had a mind to not change it because it corresponds with the Optional method but thinking again, I think returning null can never be better. I'll make the change in a followup PR, as I tried to make it this time I realized first I want to do some refactoring of the no-op segments to not generate IDs, it'd be a bit weird to generate a random ID in a getCurrent method.
| SegmentContext context = segmentContextResolverChain.resolve(); | ||
| if (context != null) { | ||
| return context.beginSubsegment(this, name); | ||
| if (context == null) { | ||
| // No context available, we return a no-op subsegment so user code does not have to work around this. Based on | ||
| // ContextMissingStrategy they will still know about the issue unless they explicitly opt-ed out. |
While in the strictest sense, changing behavior like this is a breaking change, it's generally acceptable to replace a null with a no-op (but not vice versa). The idea is that returning the no-op segment should work in any caller code in the non-null case no problem. On the flip side, there should be less chance of NPE in user code as a result - all the major Java tracing libraries make sure never to return null spans so it's fairly standard practice.
PS: I'm intentionally not using the name of the actual class here, it's something we'll need to rename for the next major version.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.