feat(difs): Expose dif location as a URI in candidates #344
Conversation
This replaces SourceFileId with ObjectFileSource and re-structures how this works to have more consistent handling of file sources and to have a more consistent place to implement URI etc.
Instructions for snapshot changesSentry runs a symbolicator integration test suite located at Follow these steps to update snapshots in Sentry:
|
src/actors/snapshots/symbolicator__actors__symbolication__tests__add_bucket-2.snap
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Michael Auer <mail@jauer.org>
This introduces a newtype for the URI to make functions stricter about what they are returning.
| /// not provide a hostname nor percent-encode. Use this only for diagnostics and use | ||
| /// [`FilesystemObjectFileSource::path`] if the actual file location is needed. | ||
| pub fn uri(&self) -> ObjectFileSourceURI { | ||
| format!("file:///{path}", path = self.path().display()).into() |
There was a problem hiding this comment.
I mean, the doc comment says its not supposed to be compliant, but do we have tests for this on windows and linux?
There was a problem hiding this comment.
Which tests do you mean?
There was a problem hiding this comment.
oh, i know what you mean :). So we only run prod on linux and we don't even run symbolicator tests on windows (I don't think it even can run there)
| } | ||
|
|
||
| /// Returns the URL from which to download this object file. | ||
| pub fn url(&self) -> Result<Url> { |
There was a problem hiding this comment.
I would prefer to make the signature Result<Url, anyhow::Error>, since it's easier to see at a glance what this returns. That said, since this is a join_url operation, can we make this return a URL-specific error? I'd rather have the caller coerce this.
There was a problem hiding this comment.
I thought our current approach was to not use custom errors unless we need to match on them?
The Result<Url> part is more just familiarity imho, once you're used to anyhow::Result it's a pretty obvious shortcut.
There was a problem hiding this comment.
So if you write anyhow::Result rather than importing, I'm also fine with it 👍
Point taken, let's go with anyhow for now. I'll propose something for this in our style guide. What I've often seen the community do, is:
- If there is a function that performs a single task, then throw the specific error for this task. For example, I/O functions should throw
std::io::Error. Parse functions should throw a custom parse error. - For propagating, libraries should create their own error type, either with a plain
ErrorKindenum or with flag getters. - For propagating, applications may use a boxed error such as anyhow as long as no error handling is needed.
src/types/mod.rs
Outdated
| /// | ||
| /// [`ObjectFileSource`]: crate::services::download::ObjectFileSource | ||
| #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)] | ||
| pub struct ObjectFileSourceURI(String); |
There was a problem hiding this comment.
nit: Would ObjectUri suffice as a shorter name?
There was a problem hiding this comment.
meh, doesn't really matter but mostly i think that's too short for such an auxiliary type
oops...
This is all very silly, but more readable. So much confusion.
* master: feat(difs): Expose dif location as a URI in candidates (#344)
This adds a "uri" field to the dif candidates info.
To do this it firstly refactors the SourceFileId into an
ObjectFileSource to provide better structure and a unified place to
add the URI.
The second commit then uses this to add to the DIF candidates. This
also needs to add insta redaction since the port number shows up in
the URI otherwise.
sentry snapshots are not affected by this as they do not enable dif candidates info.