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

Ignore exit success #91

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

parsonsmatt
Copy link
Collaborator

Fixes #90

This PR alters the inSpan''' function to ignore ExitSuccess exceptions.

Not sure if this is right 🤔

@iand675
Copy link
Owner

iand675 commented Sep 12, 2023

This is quite a funny case. I guess it's not an error per se, and it's probably usually true that it would not be a surprise to the thrower that the span would be treated as exiting successfully in this situation...

That said, it seems like you'd generally be able to throw ExitSuccess or other "positive exceptions" from outside of the inSpan'' block. If we went the direction of baking in gracefully treating some thrown exceptions as successes, I'd probably like to see SpanArguments provide a more generalized mechanism–

An example of a case that we currently deal with at Mercury is that we have some 3rd-party systems that we use http-client to long-poll. The long-polling connections always time out at 60 seconds, so they show up as errors due to receiving some kind of timeout exception, but actually that's working as expected. What if we added something along the lines of:

data SpanExceptionDecision 
  = Unexpected (HashMap Text Attribute)
  | Expected (HashMap Text Attribute)

data SpanArguments = SpanArguments
  { ...
  , diagnoseException :: SomeException -> SpanExceptionDecision
  }

@parsonsmatt
Copy link
Collaborator Author

Yeah, it has big Error: Program completed successfully vibes

diagnoseException on SpanArguments is a good idea - but it does seem a bit worrying to me that you can do, say,

inSpan "asdf" defaultSpanArguments { diagnoseException = blah } do
  inSpan "qwer" defaultSpanArguments do
    throwSomethingDope

Then the inner span is reported with an error, but the outer one isn't. Maybe that's totally fine and the behavior for exception diagnosis shouldn't be inherited. Idk on the exact semantics.

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.

Handling ExitSuccess gracefully
2 participants