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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,6 @@ This repository contains RFCs and DACIs. Lost?
- [0071-continue-trace-over-process-boundaries](text/0071-continue-trace-over-process-boundaries.md): Continue trace over process boundaries
- [0072-kafka-schema-registry](text/0072-kafka-schema-registry.md): Kafka Schema Registry
- [0073-usage-of-transaction-types](text/0073-usage-of-transaction-types.md): Usage of transaction types
- [0074-source-context-via-links](text/0074-source-context-via-links.md): Source context via links
- [0078-escalating-issues](text/0078-escalating-issues.md): Escalating Issues
- [0080-issue-states](text/0080-issue-states.md): Issue States
75 changes: 75 additions & 0 deletions text/0074-source-context-via-links.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
- Start Date: 2023-02-15
- RFC Type: feature
- RFC PR: <https://github.com/getsentry/rfcs/pull/74>
- RFC Status: draft

# Summary

In situations where a source code link and a line number is available for a frame, present the link in the UI and show
the source code surrounding the line.

# Motivation

The source context is currently composed of three [`Frame` attributes](https://develop.sentry.dev/sdk/event-payloads/stacktrace/#frame-attributes):

- `context_line` - Source code in filename at `lineno`.
- `pre_context` - A list of source code lines before `context_line` (in order) – usually [`lineno - 5:lineno`].
- `post_context` - A list of source code lines after `context_line` (in order) – usually [`lineno + 1:lineno + 5`].

Showing a source code context and a link to it (e.g. to GitHub) currently works only if the source code itself is
available during symbolication, thus the attributes are filled in at that time, or if they were already sent with the event by the SDK.

There are, however, situations where we do have a URL representing the source code contents, but not the contents itself (without downloading it), for example:

- Portable-PDB source-link (.NET, see [this `symbolic` issue](https://github.com/getsentry/symbolic/issues/735))
- [Debuginfod servers](https://www.mankier.com/8/debuginfod#Webapi-/buildid/BUILDID/source/SOURCE/FILE) (we don’t support these yet)
- SourceMaps (either embedded sourcesContent or using individual source files)
- via a repository integration in combination with associated commit

In these cases, the actual source code is not necessary to do symbolication (as opposed to source maps and other types
of obfuscated source containers) but it is useful for end-user when evaluating the issue in the UI.

## Related GH feature requests

- [Extend stack-trace linking UI](https://github.com/getsentry/sentry/issues/35608)
- [Support stack trace linking when stack frames do not contain line context.](https://github.com/getsentry/sentry/issues/44015)

# Options Considered

## A | Download the source code during symbolication

One option is to download the source code from its link at the time the event is symbolicated, filling the context-related
`Frame` attributes for each stacktrace frame, while the event is being processed.

### Pros

- Reuses existing fields (changes only needed in the `Symbolicator`).
- Sources surrounding the frame line are then stored in the database, enabling [search-by-code](https://github.com/getsentry/sentry/issues/3755).
- May be possible to do server-side authentication (based on project/org configuration) - this would complicate the solution & caching though, as opposed to only supporting publicly available sources.

### 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.

- 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?


## B | Add a source-link to the `Frame` and let the UI handle it

Another option is to extend `Frame` attributes with a new field, e.g. `source_link` and let UI download the source code
as needed or just display the link.

### 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.


### Cons

- Sources not available in the database -> can't [search-by-code](https://github.com/getsentry/sentry/issues/3755).
- Accessing private sources may be problematic. We could still show the source link, though.

# Selected Option - Combined approach

Because neither option is a clear winner, we went for a combined approach, i.e.

- Add a `source_link` field to the frame.
- Resolve source context from remote URLs, but only for in-app frames.