Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(subscriber): ignore subsequent data for spans that weren't recorded #212

Merged
merged 2 commits into from
Dec 16, 2021
Merged

fix(subscriber): ignore subsequent data for spans that weren't recorded #212

merged 2 commits into from
Dec 16, 2021

Commits on Dec 16, 2021

  1. fix(subscriber): ignore spans that weren't initially recorded 

    Currently, a strange behavior exists in the `console-subscriber`
crate. If a span for a task, async op, or resource is created, and the
event buffer is full, the aggregator task will not be informed of that
span's creation. But, if the event buffer then empties out, we might
send the aggregator task enter/exit/close events for that span. This
results in the aggregator receiving events for an unknown span, which
might result in panics or subtly wrong data.

This branch fixes this by changing the `ConsoleLayer` to track whether
it was able to successfully send an event for the creation of a span,
and to only care about subsequent events on that span if it successfully
recorded the span's creation. This is done by inserting a zero-sized
marker type into the span's extensions map, and ignoring spans that
don't have this marker.
    @hawkw
    hawkw committed Dec 16, 2021
    Configuration menu
    Copy the full SHA
    2eb6f01 View commit details
    Browse the repository at this point in the history

  2. fix(subscriber): ignore exiting spans that were never entered 

    Similarly, if we don't record entering a span due to event buffer
capacity, it doesn't make sense to record exiting it. This commit
changes the `ConsoleLayer` to only push a span to the current thread's
span stack if we were able to successfully send an `Enter` event to the
aggregator. This means that it won't be considered the parent span for
other events/spans. When a span is exited, we only send an `Exit` event
to the aggregator if the span *was* previously recorded as being entered.

In theory, ignoring subsequent events on spans that were dropped due to
buffer capacity technically means we are losing *more* data than we
would have if we did not ignore those spans. But, the data we are losing
here is *wrong*. For example, we cannot calculate a correct poll time
for a poll where we didn't record the beginning of the poll, and we only
recorded the poll ending. Therefore, I think it's better to ignore this
data than to make a half-assed attempt to record it even though we know
it's incorrect.

I believe this will probably also fix issue #180. That issue occurs
when we attempt to decrement the number of times a task has been
polled, and sometimes --- if an `enter` event for that task was missed
--- we may subtract more than we've added to the counter. By ignoring
exits for spans that we never recorded as entered, this panic should now
be avoided.
    @hawkw
    hawkw committed Dec 16, 2021
    Configuration menu
    Copy the full SHA
    dd562d7 View commit details
    Browse the repository at this point in the history