Keep track of emitted in emitter. #279
Conversation
anuraaga
force-pushed
the
use-emitted-in-emitter
branch
from
09ac6c8 to
7c9b078
Compare
Codecov Report
@@ Coverage Diff @@
## master #279 +/- ##
============================================
- Coverage 59.57% 59.26% -0.31%
+ Complexity 1210 1207 -3
============================================
Files 131 131
Lines 5046 5060 +14
Branches 586 589 +3
============================================
- Hits 3006 2999 -7
- Misses 1767 1786 +19
- Partials 273 275 +2 Continue to review full report at Codecov.
|
| @@ -216,7 +220,7 @@ protected EntityImpl(AWSXRayRecorder creator, String name) { | |||
| */ | |||
| protected void checkAlreadyEmitted() { | |||
| synchronized (lock) { | |||
| if (emitted) { | |||
| if (ended || emitted) { | |||
There was a problem hiding this comment.
For what it's worth, this does not actually make entities immutable after they're ended because ended is set in the same conditions as emitted before it. However I don't think that we should "fix" this and make them immutable, as it would break several customer setups similar to this one:
AWSXRay.beginSegment('service');
// Make an instrumented AWS SDK request that begins & ends a subsegment
DynamoClient.listTables();
// Get the Dynamo subsegment
Subsegment sub = AWSXRay.getSubsegments().get(0);
// Add some additional metadata they want to see on the Dynamo subsegment.
// Because the subsegment is created created and ended by our instrumentation logic, they can only
// access it after it's been ended
sub.putAnnotation("key", "value"); I tested locally and this type of logic was still possible even after your change (because ended is the same as emitted before).
There was a problem hiding this comment.
Yup just intending to preserve existing behavior.
There was a problem hiding this comment.
Ok just making sure, thought you were intending to change that behavior based on the PR description.
There was a problem hiding this comment.
it either is the same as the old emitted variable, or it's the same as the inProgress field.
Thanks for the suggestion, It seemed nice, but at first glance while this seems to be true, unfortunately we expose a setInProgress method :/ So technically, it's possible for a user to call setInProgress (we don't really expect it but it's possible) and mutations were still allowed while if I used inProgress in the mutation check instead of a new variable, they would stop being allowed. So I'm thinking of leaving this new field.
| @@ -216,7 +220,7 @@ protected EntityImpl(AWSXRayRecorder creator, String name) { | |||
| */ | |||
| protected void checkAlreadyEmitted() { | |||
| synchronized (lock) { | |||
| if (emitted) { | |||
| if (ended || emitted) { | |||
There was a problem hiding this comment.
Yup just intending to preserve existing behavior.
This reverts commit d66d69a.
* Revert "Don't lock segment from subsegment (#306)" This reverts commit d5ccae2. * Revert "Keep track of emitted in emitter. (#279)" This reverts commit d66d69a. * Revert "Remove subsegments lock (#278)" This reverts commit adfb395. * Revert "Lock all accesses to entities. (#250)" This reverts commit f0a3b3a. * add back errorprone annotations * fixed shouldPropagate * added back getSubsegmentsCopy * added back compareAndSetEmitted * whitespace fix
Issue #, if available:
Description of changes:
In #278, we try to avoid having a long-standing lock around all emissions in a subtree. It causes a race where subsegments may be emitted multiple times. This is because the current
emittedisn't being kept track of by the emitter actually, it's set by the caller. This PR splits intoemitted, which is intended to prevent double-emission, andended, which is intended to prevent mutation of an ended entity in most cases.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.