Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #361 +/- ##
============================================
+ Coverage 58.35% 58.41% +0.05%
- Complexity 1226 1229 +3
============================================
Files 133 133
Lines 5016 5011 -5
Branches 595 589 -6
============================================
Hits 2927 2927
+ Misses 1815 1814 -1
+ Partials 274 270 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| isSampled ? currentSubsegment.getId() : null, | ||
| isSampled ? SampleDecision.SAMPLED : SampleDecision.NOT_SAMPLED); | ||
| request.addHeader(TraceHeader.HEADER_KEY, header.toString()); | ||
| request.addHeader(TraceHeader.HEADER_KEY, TraceHeader.fromEntity( |
There was a problem hiding this comment.
This code doesn't look equivalent to the one it's replacing.
In the left side code, the parent id as the TraceHeader argument is either currentSubsegment's id or null. Only the isSampled is fetched from either previousSubsegment or the recorder.getCurrentSegment().
In the right side code, the whole TraceHeader will be created either from previousSubsegment or currentSubsegment.
There was a problem hiding this comment.
Wow, good catch! This was a bug.
I looked at the original code and I don't think we even need to use the previous subsegment here.
| this.isRecording = false; | ||
| this.samplingStrategyOverride = samplingStrategyOverride; | ||
|
|
||
| id = creator.getIdGenerator().newEntityId(); |
There was a problem hiding this comment.
If this NoOpSubsegment is never emitted and never propagated, can we set a static id here? We should avoid unnecessary computation if we can.
There was a problem hiding this comment.
Removed.
I added it when I noticed parentId was missing in NoOpSubsegments in trace headers. Then I saw we don't actually add it to the header when it is not sampled. Forgot to remove it after that.
| * @return the sampled | ||
| */ | ||
| @JsonIgnore | ||
| @Override |
There was a problem hiding this comment.
Do we need to necessarily override the isSampled() method from Entity?
Is it possible for Entity types and subtypes to directly use this method from Entity without an override?
There was a problem hiding this comment.
Good point, removed.
Issue #, if available:
Description of changes:
Problem 1
We have redundant code for creating a trace header.
Solution
Create a centralized function for creating trace headers from entities. This required NoOpSubsegment to have its traceId.
Problem 2
Why does NoOpSubsegment need a namespace and name? We have a bug where we are not correctly ending subsegments if they are NoOp. The subsegment gets created, but never ended by afterResponse because it is seen as a duplicate because its namespace and name is always empty.
Solution
Pass down the name and namespace in NoOpSubsegments.
https://github.com/aws/aws-xray-sdk-java/blob/master/aws-xray-recorder-sdk-aws-sdk/src/main/java/com/amazonaws/xray/handlers/TracingHandler.java
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.