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

rfc(feature): Source context via links #74

Merged
merged 6 commits into from
Mar 30, 2023

Conversation

vaind
Copy link
Contributor

@vaind vaind commented Feb 15, 2023

Evaluates options to support source context via source links (URLs).

Rendered RFC

@vaind vaind force-pushed the rfc/source-context-via-links branch 2 times, most recently from b613e3d to b30618a Compare February 15, 2023 12:04
@vaind vaind marked this pull request as ready for review February 15, 2023 13:07
@mattjohnsonpint
Copy link
Contributor

My vote is for option A.

It seems like with option B we'd need to think about anywhere that data might need to be rendered - which might not just be the UI. You called out search, but there could be others. Perhaps with integrations or alerts (if not now, then in the future).

@mattjohnsonpint
Copy link
Contributor

A third option (not my first choice though) would be to have Sentry CLI download the files from the links and use them to create a source bundle, then upload the source bundle through the existing mechanism.

That would address .NET SourceLink, but I'm not sure it would cover the other cases mentioned.


### Cons

- Downloads all frame sources even though the event/frame may never be viewed by a user.
Copy link
Member

Choose a reason for hiding this comment

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

Slightly related:
We should make "we want source context" configurable / an input option in symbolicator.
For example the profiling product goes through symbolicator, but does not use source context at all (they throw it away in the profiling processor AFAIK). We could potentially make profiles cheaper to symbolicate by just not doing source context resolution at all. CC @indragiek

It may also be worth considering making the lines of context configurable, as it is hardcoded to 5 right now.

### Cons

- Downloads all frame sources even though the event/frame may never be viewed by a user.
- Adds potentially tens of requests for each symbolicated event that has a source link - as many as there are in the stack trace.
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, it adds a ton of requests, and processing to stack frames that customers are very unlikely to ever look at (for example background threads). Though good caching should hopefully take care of most of this.

I’m not sure we have metrics for any of this right now? How many stack traces users are actually looking at?

### Pros

- No overhead on the server for loading sources in situations where the event/frame wouldn't be shown.
- Requests to fetch the source code, if any, are made by the user browser, thus avoiding potential quota limits.
Copy link
Member

Choose a reason for hiding this comment

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

Well, while we as a single large consumer would not run into quotas, the source server itself would get a ton more requests by individual users that are not being cached.


# Unresolved questions

- Neither approach is a clear winner, any suggestions?
Copy link
Member

Choose a reason for hiding this comment

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

I believe from an implementation POV, we can even do a hybrid approach:

  • Always attach a source link to stack frames.
  • Only download/apply source context for the top N frames of the crashing thread.

With that, we can focus the ahead-of-time work to the context the end user is most likely to actually look at, and search for; while keeping options open for the future.

I believe this is big enough to involve more people from the product side of things, to answer the following questions:

  • Which parts of the product need source context?
  • How much (lines of) source context?
  • How does this tie into search-by-code?
  • Do we need all the source context at once, or is it okay to lazily load some of it at a later time (favoring a hybrid approach)

@vaind
Copy link
Contributor Author

vaind commented Feb 17, 2023

A third option (not my first choice though) would be to have Sentry CLI download the files from the links and use them to create a source bundle, then upload the source bundle through the existing mechanism.

Yes, that occurred to me too, originally, but I've abandoned that quickly because it doesn't translate to debug info coming from dependencies (e.g. snuget) that will only be loaded on the server during symbolication

@vaind
Copy link
Contributor Author

vaind commented Mar 29, 2023

Who can review & merge this? As is, it just describes what is in production already & some future ideas

Co-authored-by: Arpad Borsos <swatinem@swatinem.de>
@Swatinem Swatinem merged commit 84be2cc into getsentry:main Mar 30, 2023
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.

3 participants