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.

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

fix(ioredis): Make sure span.end is only called once #2256

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AbhiPrasad
Copy link
Member

Which problem is this PR solving?

Fixes #2254

Short description of the changes

I moved the endSpan helper inline and made it mutate a spanHasEnded flag to enforce span.end is only called once.

Not sure how to best test this change, advice there appreciated!

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.36%. Comparing base (dfb2dff) to head (101e47f).
Report is 149 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2256      +/-   ##
==========================================
- Coverage   90.97%   90.36%   -0.62%     
==========================================
  Files         146      148       +2     
  Lines        7492     7524      +32     
  Branches     1502     1576      +74     
==========================================
- Hits         6816     6799      -17     
- Misses        676      725      +49     
Files Coverage Δ
...try-instrumentation-ioredis/src/instrumentation.ts 87.85% <75.00%> (-2.37%) ⬇️

... and 51 files with indirect coverage changes

@JacksonWeber
Copy link
Contributor

Once some tests are added, this looks good to me. As for how you might go about doing that - would it be possible to recreate the scenario with the ioredis package that was calling endSpan multiple times and then use sinon to assert that with the new changes the endSpan method is only ever called once under those conditions?

@AbhiPrasad
Copy link
Member Author

use sinon to assert that with the new changes the endSpan method is only ever called once under those conditions

I'm struggling figuring out a good way to spy on the returned span here. I have something like so:

let endSpanSpy;
const original = instrumentation['tracer'].startSpan;
sinon
  .stub(instrumentation['tracer'], 'startSpan')
  .callsFake((name, options, context) => {
    const span = original(name, options, context);
    endSpanSpy = sinon.spy(span, 'end');
    return span;
  });

but that seems to be causing issues for some reason.

@AbhiPrasad
Copy link
Member Author

Alright I have it something like so:

const endSpanSpy = sinon.spy(OtelSpan.prototype, 'end');
const span = provider.getTracer('ioredis-test').startSpan('test span');
try {
  await context.with(trace.setSpan(context.active(), span), () => {
    return client.set('non-int-key', 'non-int-value').then(() => {
      return client.incr('non-int-key', () => {
        throw new Error('This is an error');
      });
    });
  });
} catch (e) {
  // ignore
}

console.log(endSpanSpy.callCount);

but the mocha after hooks don't seem to like this 😢. My inexperience with mocha/sinon really showing here 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ioredis instrumentation span.end getting called twice
4 participants