-
-
Notifications
You must be signed in to change notification settings - Fork 62
Add a unified symbolicate-v2
endpoint
#1795
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
base: master
Are you sure you want to change the base?
Conversation
/// This currently includes minidump and apple-crashreport based on stored attachments. | ||
/// In the future, we could move all of native, js and jvm symbolication to this enum. | ||
#[derive(Deserialize)] | ||
#[serde(tag = "type", rename_all = "lowercase")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be snake_case
or kebab-case
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well be snake_case
. So far its only relevant for applecrashreport
vs apple_crashreport
. I have no strong opinion either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's fine to leave this as it is.
The aim of this endpoint is to eventually supercede all other endpoints, unifying common request fields in a top-level body, and then matching on the actual symbolication request. As a first usecase, this now supports new `objectstore`-based requests for minidump and apple crashreport processing. This can thus replace the existing multipart based endpoints, and takes the `storage_url` as part of the JSON body. It then downloads the given file, and symbolicates as usual.
13fc5a4
to
dc6b43e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks very straightforward. I'm slightly unsure about using symbolicate-v2
as the route for the new endpoint; wdyt @jjbayer? The only alternative I can come up with is symbolicate-unified
and that's probably worse.
/// This currently includes minidump and apple-crashreport based on stored attachments. | ||
/// In the future, we could move all of native, js and jvm symbolication to this enum. | ||
#[derive(Deserialize)] | ||
#[serde(tag = "type", rename_all = "lowercase")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's fine to leave this as it is.
How about |
That's better IMO. What do you think @Swatinem? |
The aim of this endpoint is to eventually supercede all other endpoints, unifying common request fields in a top-level body, and then matching on the actual symbolication request.
As a first usecase, this now supports new
objectstore
-based requests for minidump and apple crashreport processing. This can thus replace the existing multipart based endpoints, and takes thestorage_url
as part of the JSON body.It then downloads the given file, and symbolicates as usual.
This is an alternative to #1780, and avoids pulling in our
objectstore-client
.It also creates a new endpoint instead of reusing the existing multipart-based ones.
Ref FS-103