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

(PR created) Allow attachments to be bound to exception more easily #682

Closed
fzyzcjy opened this issue Dec 25, 2021 · 25 comments · Fixed by #1404
Closed

(PR created) Allow attachments to be bound to exception more easily #682

fzyzcjy opened this issue Dec 25, 2021 · 25 comments · Fixed by #1404

Comments

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Dec 25, 2021

Why I need this (very much!)

My code is like:

void main_function() {
  try {
    inner_function(); // indeed, functions calling functions calling functions... so the calling stack is deep
  } catch (e,s) {
    Sentry.captureException(e, s);
  }
}

void inner_function() {
  var my_data = ...;
  try {
    do_some_work_that_can_throw_exception(my_data);
  } catch (e,s) {
     Sentry.pleaseBindAttachmentToException(e, my_data); // <-- !!
     rethrow;
  }
}

Notice the Sentry.pleaseBindAttachmentToException, where I want to bind attachment to exception.

However, as we know, we cannot do it. I can create some code to bind extra to exception, because extra can be modified in EventProcessors (so I can manually create a static map holding some extras and bind to exception in my own EventProcessor). However, EventProcessors cannot modify or add attachments. So I have no way to do it :(

Proposed solution

Say, add a field to SentryOptions, e.g. void Function(Scope)? withScopeWhenCapture. Then, in Hub.captureException and similar places, call that callback. This is very simple and I will implement it if you like.

related: #646

Hi @marandaneto could you please have a look (since this thread is strongly related to #646)? Thanks! And @ueman could you please also have a look? Thanks!

@fzyzcjy fzyzcjy changed the title (Will PR in 1 day if needed) Add callback to SentryOptions that *can* touch scope (and thus touch attachments) (Will PR in 1 day if needed) Allow attachments to be bound to exception more easily Dec 25, 2021
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Dec 26, 2021

Hi @marandaneto @ueman could you please suggest whether this is acceptable? If so I will make a PR. This is a blocking issue on our app...

@ueman
Copy link
Collaborator

ueman commented Dec 26, 2021

If I'm not mistaken you can already do something like this:

Sentry.captureException(exception: ex, withScope:(scope) {
    scope.addAttachment(attachment);
});

I'm not sure if that already works, but you might try that. If not @marandaneto probably appreciates a PR which enable scoped attachments. I'm not a maintainer of this SDK anymore.

Currently is also Christmas season, so most people are probably out of office.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Dec 26, 2021

@ueman No I cannot do that...Notice that I captureException in a outer function, while I addAttachment in a inner function. For example, outer_function() calls func1() which calls func2() which calls func3(), ..., which calls func100() which calls inner_function().

Currently is also Christmas season, so most people are probably out of office.

Ah I did not realized that (I have been doing my project for 2 years without using holidays ;) ). Sorry for disturbing.

@ueman
Copy link
Collaborator

ueman commented Dec 26, 2021

Oh I see. In that case the solution would probably be similar to .NETs local scopes. Dart hasn't gotten support for that yet but that would probably the solution for your problem.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Dec 26, 2021

@ueman Well that is a bit the inverse. In my case I want to bind attachment in inner function and capture exception in outer. With the proposed modification, that can be implemented trivially.

@ueman
Copy link
Collaborator

ueman commented Dec 26, 2021

Yeah, with mine, too. The code would be something like this:

void main_function() {
Sentry.withScope((scope) {
// within this scope, the attachment is added to the capture exception
  try {
    inner_function(); // indeed, functions calling functions calling functions... so the calling stack is deep
  } catch (e,s) {
    Sentry.captureException(e, s);
  }
}
// here the attachment isn't added to the exception anymore
Sentry.captureException(ex); // this exception has no attachment
}

void inner_function() {
  var my_data = ...;
  try {
    do_some_work_that_can_throw_exception(my_data);
  } catch (e,s) {
     Sentry.addAttachment(my_data);
     rethrow;
  }
}

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Dec 26, 2021

Agree. Since that is a bit complex (read: can introduce bugs), I will firstly implement a simpler solution

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Dec 26, 2021

PR made: #683

@fzyzcjy fzyzcjy changed the title (Will PR in 1 day if needed) Allow attachments to be bound to exception more easily (PR created) Allow attachments to be bound to exception more easily Dec 26, 2021
@marandaneto
Copy link
Contributor

I'm on vacay, maybe @bruno-garcia can help

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Jan 5, 2022

@marandaneto No problem, I can wait for it

@marandaneto
Copy link
Contributor

I'll discuss that internally and get back to you, but I agree with @ueman , the solution could be using local scopes, it'd be confusing to have a beforeSend callback without Scope, and a beforeCaptureWithScope, our public APIs are aligned and documented https://develop.sentry.dev/sdk/unified-api/ so we'd need a bit of discussion before going forward.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Jan 10, 2022

@marandaneto Sure. I have used it in my app in production and my implementation seems quite good.

@bruno-garcia
Copy link
Member

bruno-garcia commented Feb 7, 2022

You could solve this today by throwing a new exception that holds the reference to your attachment and the inner exception.

pseudocode:

void main_function() {
  try {
    inner_function(); // indeed, functions calling functions calling functions... so the calling stack is deep
  } catch (e,s) {
    if (e is AttachmentException) {
      attachEx - (AttachmentException)e;
      Sentry.captureException(attachEx.Exception, attachEx.Stacktrace, scope: (s) => s.addAttachment(attachEx.Attachment));
    }
  }
}

void inner_function() {
  var my_data = ...;
  try {
    do_some_work_that_can_throw_exception(my_data);
  } catch (e,s) {
     throw new AttachmentException(e,s,my_data);

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Feb 8, 2022

@bruno-garcia Sounds interesting, thanks

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Mar 3, 2022

I'll discuss that internally and get back to you

Hi, is there any updates?

@marandaneto
Copy link
Contributor

I'll discuss that internally and get back to you

Hi, is there any updates?

Yes, but we're going to experiment on sentry-java first, which is using hints as the way to hold and filter attachments before being sent.
getsentry/sentry-java#1929

You could e.g. Sentry.captureException(exception, ["attachments": myAttachment]) or something similar to that, I will update this issue once we have a design for the API.
You'll be able to remove an attachment using the beforeSend as well via hints, so #711 would have a workaround as well.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Mar 3, 2022

Thanks for the update! As long as the goal mentioned in the first post of the issue is solved it, any method is great

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 13, 2022

@marandaneto Hi is there any updates?

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 13, 2022

You could solve this today by throwing a new exception that holds the reference to your attachment and the inner exception.

pseudocode:

void main_function() {
  try {
    inner_function(); // indeed, functions calling functions calling functions... so the calling stack is deep
  } catch (e,s) {
    if (e is AttachmentException) {
      attachEx - (AttachmentException)e;
      Sentry.captureException(attachEx.Exception, attachEx.Stacktrace, scope: (s) => s.addAttachment(attachEx.Attachment));
    }
  }
}

void inner_function() {
  var my_data = ...;
  try {
    do_some_work_that_can_throw_exception(my_data);
  } catch (e,s) {
     throw new AttachmentException(e,s,my_data);

Btw this has a big drawback: the exception type is changed. e.g. if outer function only knows to catch MyException, it will not look at AttachmentException at all.

@marandaneto
Copy link
Contributor

@fzyzcjy yes, we are experimenting with a new API for hints on the Java repo and this will likely be the solution here as well if it works well. getsentry/sentry-java#2045
Via hints you'll be able to add and remove attachments for specific events.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 13, 2022

Looking forward to it!

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Sep 11, 2022

@marandaneto Hi is there any updates? Thanks

@marandaneto
Copy link
Contributor

Yeah the approach taken on sentry-java worked well, now it's a matter of doing it on Dart/Flutter, not planning on this Quarter tho.

@marandaneto
Copy link
Contributor

#1136 is going to unblock this via hints

@marandaneto
Copy link
Contributor

It's now possible via hints.
We should introduce a new property in the Hint class that takes attachments, so you can add/remove them on the fly either when capturing events or thru beforeSend in the hints param.

@marandaneto marandaneto added this to the 7.x milestone Mar 21, 2023
@denrase denrase mentioned this issue Apr 25, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants