Skip to content

Figure out how to implement hints #5036

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

Closed
adinauer opened this issue May 4, 2022 · 5 comments
Closed

Figure out how to implement hints #5036

adinauer opened this issue May 4, 2022 · 5 comments

Comments

@adinauer
Copy link
Member

adinauer commented May 4, 2022

Problem Statement

Ideally, hints are aligned across SDKs.

A big part of the discussion is probably how to deal with attachments.

Use cases for hints:

  • Add attachments in case of a certain type of error where the error contains a path to a file to attach (Add the ability to attach on hard crash only sentry-java#1467).
  • Attach something to a specific event (not every event of a certain type but just a one-off thing) when calling captureEvent, like a one-off attachment
  • Remove an attachment to not be sent alongside the current event (it should be sent alongside future events, so not completely removed from scope)
  • Being able to extend hints without a breaking change.
  • ... Please add your use case here

The Java SDK has moved from an Object to a Map<String, Object> for hints here getsentry/sentry-java#1929 . We could build on top of this by adding our custom version of Map that has some helper methods on it, to make it easier to e.g. add attachments. Using some Util would do something similar but developers have to learn about the utils existence whereas they could have their IDE suggest how to do certain things via the helpers on our version of the Map.

A different approach would be to pass in an object with the methods we need and hide the map inside. This way we wouldn't pollute our hint API with things a Map can do but are not important to hints.

Another use case that was mentioned is to allow developers to store their custom data in hints to have access to it later (beforeSend / eventProcessor).

Closely related: #4996

Solution Brainstorm

Possible implementation that comes to mind:

  1. Add attachments from scope (and wherever else they may come frome) to a hint (attachments may already be in the hint as passed in via captureEvent - Add the ability to attach on hard crash only sentry-java#1467
  2. The attachments in the hint may be added to or removed from in beforeSend or eventProcessor
  3. At the end of captureEvent attachments that are in the hint should be sent (in case the event itself wasn't dropped)
@lobsterkatie
Copy link
Member

lobsterkatie commented May 9, 2022

A few pieces of feedback here:

  • It feels like we've kinda outgrown the name hint, and putting attachments in there doesn't make that better. In JS it's just a positional argument, so we can technically call it anything we want without breaking users, but I'm guessing there are languages where it's actually a named property, so I recognize that I'm probably just editorializing here. That said...

  • Unless there are reasons this isn't feasible for some SDKs, I'd very much rather have the attachment travel through the event pipeline attached to the event object, because I think it's significantly more intuitive for users. Currently, any manipulation users do in event processors or beforeSend is manipulation of the event, not the hint, but this would break that. In a user's mind, the attachment is attached to the event, the thing being sent, so that's where I think they would expect to look for it. Also, in languages like JS, beforeSend(event) is a perfectly valid signature, and anyone with that signature wouldn’t even have access to attachments without changing that. (Which they clearly could do, but why create another needless bit of friction, however tiny?)

  • I think this is a good point:

    This way we wouldn't pollute our hint API with things a Map can do but are not important to hints.

  • As far as users attaching custom data to hints, when in the lifecycle of an event would the user have access to the hint but not the event itself?

  • The rest seems totally reasonable.

(Thanks to @AbhiPrasad for being the one to originally surface the second point, which I feel more strongly about the more I think about it. Just typing out that paragraph made me want to go back and add the very into I'd very much rather.)

@timfish
Copy link
Collaborator

timfish commented May 10, 2022

I agree with Katie that it feels like everything should be in the event object.

Here @philipphofmann said:

the idea was that events can survive a serialization roundtrip

Is there any specific reason for this?

While it's convenient that event can currently be serialised straight into the event envelope, that probably shouldn't be a concern of users of the API.

The only downside I can see is if Relay rejects envelopes with unknown/invalid properties. We'd need tests to ensure random properties dont sneak through into the event payload.

@philipphofmann
Copy link
Member

Is there any specific reason for this?

I think that is the reason itself. Maybe @bruno-garcia or @marandaneto, can you elaborate?

@marandaneto
Copy link

Is there any specific reason for this?

I think that is the reason itself. Maybe @bruno-garcia or @marandaneto, can you elaborate?

@bruno-garcia talked about it with @mitsuhiko probably a while ago, so I am not aware either.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2022

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants