Sending or not unfinished Spans

[marandaneto]

on Java/.NET we don't discard Spans that are not finished, we send as it is, but Python discards them.
Relay drops unfinished spans so we remove them from a transaction before sending.

https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/hub.py#L497-L505

what are the trade-offs here? what would be the right approach? I'd prefer to unify this behavior, either on Python or Mobile SDKs.

In the beginning, I wanted to finish all spans automatically when the transaction.finish was called, but this was likely hiding bugs on the client's code (finishing without status? meh), so we've decided to send it as it is.

cc @bruno-garcia @untitaker @Tyrrrz @maciejwalkowiak @brustolin @rhcarvalho

[untitaker]

What is the end timestamp of an unfinished span? If you don't send one, the transaction is schematically invalid.

[marandaneto]

What is the end timestamp of an unfinished span? If you don't send one, the transaction is schematically invalid.

no end timestamp, and the reason was exactly that, at least the user knows something is wrong and they have to fix it.
not sending at all means, did it ever start? but again, it's a trade-off

[untitaker]

the transaction will be discarded if sent like that right now

[marandaneto]

the transaction will be discarded if sent like that right now

interesting, during ingestion or hidden by UI? I recall seeing unfinished spans into Sentry.

[untitaker]

During ingestion. This is the code: https://github.com/getsentry/relay/blob/6dfccae7f6501317e3b741f52b3714beab1484d0/relay-general/src/store/transactions.rs#L118-L123

[marandaneto]

During ingestion. This is the code: https://github.com/getsentry/relay/blob/6dfccae7f6501317e3b741f52b3714beab1484d0/relay-general/src/store/transactions.rs#L118-L123

can you confirm that if a transaction has N spans and 1 single span is unfinished, the whole transaction is discarded? if so, I'd fix this on relay though, it'd be fine to discard unfished spans but not the whole transaction I believe

[brustolin]

Just to contribute with the discussion, as a user I would like to know in the UI that I have some unfinished spans. I think it would be easier to troubleshoot this error instead of totally ignore the spans.

[untitaker]

yup exactly

we can discard those spans and add an error message on the transaction, however ultimately this is an SDK bug to me

[rhcarvalho]

however ultimately this is an SDK bug to me

Could also be usage error.

@marandaneto as a data point, IIRC, in OpenTelemetry if you don't finish a span it is also not sent anywhere.
We could change Relay to be more lenient and update the UI to notify users about the data problem, but that has a downside of special-casing finishing a transaction vs finishing a span. When you finish a span, you don't necessary expect all children to be finished recursively. If finishing a transaction (in other words, a top-level parent span) reports unfinished spans, that would be inconsistent.

Just an idea, if we consider this error to be worth reporting, we could use and existing API... CaptureException. Some SDKs report usage errors as error events.

[untitaker]

I would not send this volume of errors to sentry since the user will pay for them

…

On Tue, Feb 16, 2021 at 8:13 PM Rodolfo Carvalho ***@***.***> wrote: however ultimately this is an SDK bug to me Could also be usage error. @marandaneto <https://github.com/marandaneto> as a data point, IIRC, in OpenTelemetry if you don't finish a span it is also not sent anywhere. We could change Relay to be more lenient and update the UI to notify users about the data problem, but that has a downside of special-casing finishing a transaction vs finishing a span. When you finish a span, you don't necessary expect all children to be finished recursively. If finishing a transaction (in other words, a top-level parent span) reports unfinished spans, that would be inconsistent. Just an idea, if we consider this error to be worth reporting, we could use and existing API... CaptureException. Some SDKs report usage errors as error events. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#274 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGMPRMKLVKVNRJQQUVKJVLS7K7VNANCNFSM4XWDD55Q> .

[marandaneto]

yep, we'll fix on the SDKs

[marandaneto]

@untitaker do you know by chance what should we do?

quoting @maciejwalkowiak

getsentry/sentry-java#1262 (comment)

[untitaker]

When a parent span is not finished, but his child spans are finished, should the parent be removed with children or just a parent and children should stay?

Right now python sdk does not send any unfinished spans, and sends finished spans, this is irrespective of hierarchy. So in this case you likely end up with parent_span_ids that point to nowhere. This is accepted by the server.

I think as first action step we could relax the requirements in Relay such that invidivual spans with missing end timestamp are discarded instead of the entire transaction. It isn't hard to fix, I would welcome a PR ;)

[untitaker]

cc @jan-auer for discussion of transaction schema behavior

[lobsterkatie]

If we do start showing users a UI warning about unfinished spans, we're going to first have to do a thorough audit to make sure we're not the ones causing that error anywhere. Right now, it's not something we guard against super hard, because such spans are silently dropped. I'd have to check for sure, but I believe there are spots at least in the JS SDK where it's not unusual for not all spans to finish.

[bruno-garcia]

With today's server behavior spans must be finished for the transaction to be accepted.
This means we can lose these spans that finish slightly after a transaction, but a potential solution for that will require some thinking and changes in to SDKs/Protocol/Relay.