Rewrite NewRelicTrace to support fiber stops/starts #5240
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Basically, using
do ... end
-type instrumentation can get confused when there's aFiber.yield
inside the block. (Dataloader does this when a field calls.load
.) You can end up with situations where the sum of segments adds up to more than the total request time, because Fiber wait time is being added to the time when another Fiber is running. This makes sense-ish from New Relic's standpoint; they can't really know when the application handed over control in a way that "shouldn't count." See discussion at newrelic/newrelic-ruby-agent#3026.So, what we can do is stop spans when there's a
.yield
called, then create a new span after the Fiber resumes control. I developed this in PerfettoTrace and I intend to roll it out to the other tracers, too.I ran a little test app locally to confirm that this addresses the problem. Here's a before-after where the time tracking starts to line up:
TODO:
Also do Accept Distributed Trace Headers From New Relic #5078Still not clear on this