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

Enable tighter attribution of exceptions to spans #1380

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

Twisol
Copy link
Contributor

@Twisol Twisol commented Mar 29, 2024

  • Tickets addressed: N/A
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

This work was done while pairing with @mattdailis. As Matt describes it:

While debugging #659, we discovered that an exception thrown inside of a ModelActions.scoped() call causes that span to be closed, which means that the simulation engine cannot tell which span resulted in the exception. While this was not exactly a blocker - we're still able to recover the directive id - we felt that this is an unfortunate property of the new span semantics defined in #1174. This PR adjusts those semantics to require that a new span always be paired with a new task (adding the callWithSpan and spawnWithSpan methods), which means that any exception originating in a span will be more readily available to the simulation engine.

This PR preserves the one-to-many relationship between tasks and spans that is key to the memory gains of #1174. Morally, it just makes it so that scoped() accepts a Task(Factory) rather than a mere Runnable, which gives us finer-grained visibility into when exceptions cross span boundaries. A task is "just" a simulation-aware runnable, anyway!

As a bonus, because of this increased visibility into the model, we were able to remove the somewhat convoluted logic surrounding shadowedSpans. This is because every span is created for some task (even if more are attached to it later), so it is now simply impossible for a task to enter a span other than the one it was spawned into.

Verification

This PR is opened from a branch in the Aerie repo, so we can actually watch the tests pass or fail!

  • @mattdailis will also do a sanity check with the TT6 plan.

Documentation

The ModelActions.scoped() method has been replaced by ModelActions.callWithSpan(), along with a small menagerie of similar methods.

Future work

Ideally, reporting the span in which an exception occurred would allow finer-grained debugging when attempting to suss out the source of an issue. This is also in-line with a future intention to propagate exceptions (and return values) from a called task to its caller.

@Twisol Twisol requested a review from a team as a code owner March 29, 2024 03:57
@Twisol Twisol requested review from joswig and JoelCourtney March 29, 2024 03:57
@mattdailis
Copy link
Collaborator

Thank you for looking into this so quickly - I will run some local tests on this in the morning to verify that it works as advertised. In addition to addressing our proximate issue (associating exceptions with spans), I think this is a simpler mental model than the pushSpan/popSpan interface.

@mattdailis mattdailis force-pushed the feature/exception-attribution branch from 1f53fca to c90c822 Compare March 29, 2024 04:49
@mattdailis mattdailis requested review from mattdailis and removed request for joswig and JoelCourtney March 29, 2024 04:50
@mattdailis mattdailis merged commit 9546256 into develop Mar 29, 2024
6 checks passed
@mattdailis mattdailis deleted the feature/exception-attribution branch March 29, 2024 22:32
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.

2 participants