Add error information and filtering to spans

[cuchi]

Problem Statement

In the context of traces/transactions/spans:

Spans are being tagged with the internal_error status when either a global unhandledRejection or error happen. This is expected, but we don't have the error information in the faulty span.

In our case, we have some filters for errors that should be ignored (with the ignoreErrors option), but transactions that fail because of these errors are still being tagged with internal_error and triggering alerts that we created to track failure rates.

• The error logic for spans doesn't use the ingoreErrors option
• We can't filter the spans programmatically because the error information is missing from the span
• We can't filter the spans in the alert queries for the same reason

In the current state, we just have a bunch of failed transactions that point to no error and just render our failure rate (%) alert useless.

Solution Brainstorm

I tried to actively use the ignoreErrors logic when capturing these errors: #13084
But the solution breaks some of the scope between integrations.

A better solution (IMHO), would be to just capture the error information inside the span object: (here)

function errorCallback(error: any): void {
  const activeSpan = getActiveSpan();
  const rootSpan = activeSpan && getRootSpan(activeSpan);
  if (rootSpan) {
    const message = 'internal_error';
    DEBUG_BUILD && logger.log(`[Tracing] Root span: ${message} -> Global error occured`);
    rootSpan.setStatus({ code: SPAN_STATUS_ERROR, message });
    rootSpan.setError(error); // Here
  }
}

...so we're able to do what we want with that info:

beforeSendSpan(span) {
  if (whatever(span.error)) {
    return null;
  }
  return span;
},

[Lms24]

Hi @cuchi thanks for opening this issue! I looked at your PR as well and I agree with closing it for the moment.

Now, your suggestion to get from a span to an error generally makes sense to but unfortunately we can't add methods to the Span interface (or class). This interface has to remain consistent with the OpenTelemetry Span interface because spans emitted from the Node SDK are created by OpenTelemetry under the hood.
We also can't store the error on span attributes because attribute values can only be primitive values.

So I thought a bit about if we can turn around your logic to solve this outside the SDK and I think I came up with an (admittedly slightly hacky) solution:

const spansWithError = new Set();

Sentry.init({
  beforeSend(event) {
    if (_possiblyShouldDropEvent(event)) {
      return null;
    }

    // If we end up here, we did not drop the event
    // so let's remember the root span that should have an erroneous status
    const activeSpan = Sentry.getActiveSpan();
    const rootSpan = activeSpan && Sentry.getRootSpan(activeSpan);
    if (rootSpan) {
      spansWithError.add(Sentry.spanToJSON(rootSpan).span_id);
    }

    return event;
  },

  beforeSendTransaction(transaction) {
    const spanId = transaction.contexts.trace.span_id;
    if (!spansWithError.has(spanId)) {
      transaction.contexts.trace.status = 'ok';
    } else {
      // clean up the set to avoid memory leaks
      spansWithError.delete(spanId);
    }

    return transaction;
  },
});

Do you think this would solve your use case of avoiding to send spans with an error status where the error event was dropped?

If my suggestion doesn't work for you, there would probably be a way to emit a callback within the SDK when we drop an event that you could subscribe to to adjust the span status. However, for now, we'd like to avoid expanding the public API to the SDK if possible.

Note: I replaced beforeSendSpan with beforeSendTransaction compared to your example. The reason is that beforeSendSpan is only called for child spans but not for the root span of the span tree (what we used to call transaction before). The root span is only piped through the beforeSendTransaction callback. I fully understand that this is confusing and needs better documentation. Coincidentally, this came up before and I opened a PR to update our documentation with this limitation.

[cuchi]

Hey @Lms24!

I just tested and validated your hack, it works!
It's not the prettiest solution though (the problem isn't pretty too, I must admit), but it does the trick, and it doesn't look like something we have to change in the near future, LGTM.

Thanks for looking into that, I'm closing this issue.

[Lms24]

Great to hear, thanks for letting us know! Yup, I fully agree that this isn't pretty. If it comes up again we can think about a cleaner solution. For now I'll leave this closed.