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

feat(tracing): Don't allow missing orphan spans to be parents or be finished #636

Closed

Conversation

relaxolotl
Copy link
Contributor

Sometimes it's possible to have orphan spans hanging around due to transactions inadvertently being finished before all of its child spans are complete. This adds in handling if we're able to detect those scenarios, preventing those orphan spans from causing cascading failures. The SDK swallows and deallocs such spans.

related to #601
picks up off of #635

@relaxolotl relaxolotl force-pushed the tracing/omit-unfinished-spans branch from 9e9ed29 to 8f8ee90 Compare December 21, 2021 08:33
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit more validation than we should be doing in the SDK.
From the data model, the spans is just a flat list, and the hierarchy is then reconstructed either on the server, or even the UI. Not sure how much validation happens in later steps, but the SDK might not be the right place for that. Please check with other team members however.

@flub
Copy link
Contributor

flub commented Dec 22, 2021

i also think this might not be needed if we don't receive spans until they are finished as is already suggested in an earlier PR. Because we'd just drop the orphaned span at the time it is being finished (the transaction ID would not match).

@relaxolotl relaxolotl marked this pull request as draft December 23, 2021 06:42
@relaxolotl
Copy link
Contributor Author

full responsibility is now being placed on users to manage spans and to drop them appropriately if spans are being finished after a transaction is finished.

@relaxolotl relaxolotl closed this Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants