-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ref(getting-started-docs): Source sdks version from sentry release registry #54675
ref(getting-started-docs): Source sdks version from sentry release registry #54675
Conversation
static/app/api.tsx
Outdated
@@ -275,6 +279,7 @@ export class Client { | |||
this.baseUrl = options.baseUrl ?? '/api/0'; | |||
this.headers = options.headers ?? Client.JSON_HEADERS; | |||
this.activeRequests = {}; | |||
this.credentials = options.credentials ?? 'include'; |
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.
This was creating a problem related to accessing data from another source (cross-origin issue) while trying to get information from the sentry release registry. With this update, we can now pass a custom credentials
as this PR does for the getting-started-docs
, passing omit
sentry = "0.31.5" | ||
sentry = "${sourcePackageRegistries?.data?.['sentry.rust'] ?? '0.30.0'}" |
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 "0.31.5"?
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.
oh yes, I went through this list 2x but missed this one. Thanks 🙏
const {isLoading, data} = useApiQuery<ReleaseRegistrySdk>( | ||
['https://release-registry.services.sentry.io/sdks'], | ||
{ | ||
staleTime: Infinity, | ||
}, | ||
API_CLIENT | ||
); |
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.
Do we have an OOTB error handling here? We should get a sentry error when this request fails so that we know that versions in the onboarding are defaulting to hardcoded ones (those might be ancient at that point in time).
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.
it is a good idea, but if this request fails, sentry should capture it OOTB or? I am not aware of places where we handle isError
, other than in a few try...catch
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.
sentry should capture it OOTB or?
yeah, that's what I'm wondering as well because I don't know by heart if we capture it by default these days
it's pretty important that we log this, otherwise, we won't notice this error is happening and people will just install old versions of the SDK - is there some way we can validate whether we log it or not? just for peace of mind
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.
you are right... unfortunately we our react-query
layer still doesn't capture a request fail error
sentry = "0.31.5" | ||
sentry = "${sourcePackageRegistries?.data?.['sentry.rust'] ?? '0.30.0'}" |
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.
oh yes, I went through this list 2x but missed this one. Thanks 🙏
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
3797eff
to
51a106f
Compare
useEffect(() => { | ||
if (releaseRegistrySdk.error) { | ||
handleXhrErrorResponse( | ||
'Failed to fetch sentry release registry', | ||
releaseRegistrySdk.error | ||
); | ||
} | ||
}, [releaseRegistrySdk.error]); |
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.
this is meh but according to our slack discussion, we don't capture response errors out of the box as it was too noisy before and when we need to track them, we shall use handleXhrErrorResponse
sourcePackageRegistries, | ||
}: Partial< | ||
Pick<ModuleProps, 'dsn' | 'sourcePackageRegistries'> | ||
> = {}): LayoutProps['steps'] => [ |
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 wish we could simplify this. It's quite clunky, especially because we repeat it in lots of places.
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.
yes, I talked to Arthur about it too... we should definitely simplify this, but would prefer to do that in a separate PR
? new Error(message, { | ||
cause: { | ||
responseText: err.responseText, | ||
responseJSON: err.responseJSON, | ||
status: err.status, | ||
statusText: err.statusText, | ||
name: err.name, | ||
stack: err.stack, | ||
}, | ||
}) |
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.
This should ideally be a separate PR - I have very little confidence reviewing this peace. Do we know which field is causing troubles? The only difference I can see is cause
,message
vs responseJSON
.
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.
The main benefit of having this "layer" between the frontend and the release registry is the cache as mentioned above. A process runs every 5 minutes to get the list from the registry, but is this really a big advantage? We're also using caching in the UI with React Query.
I think it's just so we have a single way of grabbing this data - but since it requires more backend changes not worth the effort IMO.
We can keep iterating on these views after merge!
I am not sure if the problem is because we pass I also asked Billy but he is not sure why this is not working |
PR reverted: 0f1c8ff |
We're getting the SDK version from the sentry release registry and showing it for platforms that suggest a specific version for users to install.
It will be quite fast, but if the endpoint takes a bit longer to respond, a small '...loading' text will appear instead of a version number. During this time, users can't manually copy the code, and the button to copy it to the clipboard won't show up.
closes: #53921
Preview:
Screen.Recording.2023-08-17.at.14.57.14.mov
Related required PRs: