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.

Sign up for GitHub

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

Jump to bottom

ref(getting-started-docs): Source sdks version from sentry release registry #54675

Merged
merged 17 commits into from
Aug 17, 2023
Merged

ref(getting-started-docs): Source sdks version from sentry release registry #54675

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
48b3f36
ref(getting-started-docs): Source sdks version from sentry release re…
priscilawebdev Aug 14, 2023
81f6537
Merge branch 'master' into priscila/feat/getting-started-docs-source-…
priscilawebdev Aug 14, 2023
f2d7b9f
make prop optional
priscilawebdev Aug 14, 2023
7a9d428
add description to hook
priscilawebdev Aug 14, 2023
51a106f
Update static/app/gettingStartedDocs/rust/rust.tsx
priscilawebdev Aug 14, 2023
23db940
Merge branch 'master' into priscila/feat/getting-started-docs-source-…
priscilawebdev Aug 14, 2023
b77a116
cr feedback
priscilawebdev Aug 14, 2023
6caa12d
Merge branch 'master' into priscila/feat/getting-started-docs-source-…
priscilawebdev Aug 17, 2023
3a38a69
cr feedback
priscilawebdev Aug 17, 2023
ec5d7c7
cr feedback
priscilawebdev Aug 17, 2023
0f05f3c
remove test code
priscilawebdev Aug 17, 2023
5a54ead
fix tsc error
priscilawebdev Aug 17, 2023
d063924
Merge branch 'master' into priscila/feat/getting-started-docs-source-…
priscilawebdev Aug 17, 2023
1f34c19
cr feedback
priscilawebdev Aug 17, 2023
eea3f51
revert new changes
priscilawebdev Aug 17, 2023
8d91175
Merge branch 'master' into priscila/feat/getting-started-docs-source-…
priscilawebdev Aug 17, 2023
bd3397b
Matej cr feedback
priscilawebdev Aug 17, 2023
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
9 changes: 7 additions & 2 deletions static/app/api.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,10 @@ type ClientOptions = {
* The base URL path to prepend to API request URIs.
*/
baseUrl?: string;

/**
* Credentials policy to apply to each request
*/
credentials?: RequestCredentials;
Copy link
Member

Choose a reason for hiding this comment

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

can we split this out into a diff PR?

This has a bit of security implications here so we probably want to review this on it's own

/**
* Base set of headers to apply to each request
*/
Expand All @@ -265,6 +268,7 @@ export class Client {
baseUrl: string;
activeRequests: Record<string, Request>;
headers: HeadersInit;
credentials?: RequestCredentials;

static JSON_HEADERS = {
Accept: 'application/json; charset=utf-8',
Expand All @@ -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';
Copy link
Member Author

@priscilawebdev priscilawebdev Aug 14, 2023 •
edited
Loading

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

}

wrapCallback<T extends any[]>(
Expand Down Expand Up @@ -459,7 +464,7 @@ export class Client {
method,
body,
headers,
credentials: 'include',
credentials: this.credentials,
signal: aborter?.signal,
});

Expand Down
25 changes: 22 additions & 3 deletions static/app/components/codeSnippet.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Can we also split this into its own PR, would like to get @vuluongj20's eyes on this

Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import styled from '@emotion/styled';
import Prism from 'prismjs';

import {Button} from 'sentry/components/button';
import LoadingIndicator from 'sentry/components/loadingIndicator';
import {IconCopy} from 'sentry/icons';
import {t} from 'sentry/locale';
import {space} from 'sentry/styles/space';
Expand All @@ -15,6 +16,10 @@ interface CodeSnippetProps {
dark?: boolean;
filename?: string;
hideCopyButton?: boolean;
/**
* Weather the code snippet or parts of it, it is currently being loaded
*/
loading?: boolean;
onCopy?: (copiedCode: string) => void;
/**
* Fired when the user selects and copies code snippet manually
Expand All @@ -31,6 +36,7 @@ export function CodeSnippet({
onCopy,
className,
onSelectAndCopy,
loading,
}: CodeSnippetProps) {
const ref = useRef<HTMLModElement | null>(null);

Expand Down Expand Up @@ -73,7 +79,7 @@ export function CodeSnippet({
<Wrapper className={`${dark ? 'prism-dark ' : ''}${className ?? ''}`}>
<Header hasFileName={!!filename}>
{filename && <FileName>{filename}</FileName>}
{!hideCopyButton && (
{!hideCopyButton && !loading && (
<CopyButton
type="button"
size="xs"
Expand All @@ -90,13 +96,15 @@ export function CodeSnippet({
</Header>

<pre className={`language-${String(language)}`}>
<code
<Code
ref={ref}
className={`language-${String(language)}`}
onCopy={onSelectAndCopy}
isLoading={loading}
>
{children}
</code>
</Code>
{loading && <Loading mini />}
</pre>
</Wrapper>
);
Expand Down Expand Up @@ -159,3 +167,14 @@ const CopyButton = styled(Button)`
opacity: 1;
}
`;

const Code = styled('code')<{isLoading?: boolean}>`
visibility: ${p => (p.isLoading ? 'hidden' : 'visible')};
`;

const Loading = styled(LoadingIndicator)`
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
position: absolute;
`;
5 changes: 5 additions & 0 deletions static/app/components/onboarding/gettingStartedDoc/sdkDocumentation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {useEffect, useState} from 'react';

import LoadingIndicator from 'sentry/components/loadingIndicator';
import {useSourcePackageRegistries} from 'sentry/components/onboarding/gettingStartedDoc/useSourcePackageRegistries';
import {ProductSolution} from 'sentry/components/onboarding/productSelection';
import {PlatformKey} from 'sentry/data/platformCategories';
import type {Organization, PlatformIntegration, Project, ProjectKey} from 'sentry/types';
Expand All @@ -17,6 +18,7 @@ type SdkDocumentationProps = {

export type ModuleProps = {
dsn: string;
sourcePackageRegistries: ReturnType<typeof useSourcePackageRegistries>;
activeProductSelection?: ProductSolution[];
newOrg?: boolean;
organization?: Organization;
Expand All @@ -33,6 +35,8 @@ export function SdkDocumentation({
organization,
projectId,
}: SdkDocumentationProps) {
const sourcePackageRegistries = useSourcePackageRegistries();

const [module, setModule] = useState<null | {
default: React.ComponentType<ModuleProps>;
}>(null);
Expand Down Expand Up @@ -103,6 +107,7 @@ export function SdkDocumentation({
platformKey={platform?.id}
organization={organization}
projectId={projectId}
sourcePackageRegistries={sourcePackageRegistries}
/>
);
}
6 changes: 6 additions & 0 deletions static/app/components/onboarding/gettingStartedDoc/step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ type ConfigurationType = {
* The language of the code to be rendered (python, javascript, etc)
*/
language?: string;
/**
* Whether or not the configuration is currently being loaded
*/
loading?: boolean;
/**
* A callback to be invoked when the configuration is copied to the clipboard
*/
Expand Down Expand Up @@ -79,6 +83,7 @@ function getConfiguration({
additionalInfo,
onCopy,
onSelectAndCopy,
loading,
}: ConfigurationType) {
return (
<Configuration>
Expand All @@ -89,6 +94,7 @@ function getConfiguration({
language={language}
onCopy={onCopy}
onSelectAndCopy={onSelectAndCopy}
loading={loading}
>
{language === 'javascript'
? beautify.js(code, {indent_size: 2, e4x: true})
Expand Down
33 changes: 33 additions & 0 deletions static/app/components/onboarding/gettingStartedDoc/useSourcePackageRegistries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import {Client} from 'sentry/api';
import {useApiQuery} from 'sentry/utils/queryClient';

type ReleaseRegistrySdk = Record<
string,
{
canonical: string;
main_docs_url: string;
name: string;
package_url: string;
repo_url: string;
version: string;
}
>;

// This exists because /extensions/type/search API is not prefixed with
// /api/0/, but the default API client on the abstract issue form is...
const API_CLIENT = new Client({baseUrl: '', headers: {}, credentials: 'omit'});

export function useSourcePackageRegistries() {
const {isLoading, data} = useApiQuery<ReleaseRegistrySdk>(
['https://release-registry.services.sentry.io/sdks'],
{
staleTime: Infinity,
},
API_CLIENT
);
Copy link
Member

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

Copy link
Member Author

@priscilawebdev priscilawebdev Aug 14, 2023 •
edited
Loading

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

Copy link
Member

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

Copy link
Member Author

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


return {
isLoading,
data,
};
Copy link
Member

Choose a reason for hiding this comment

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

Why not just return the entire response from useApi?

priscilawebdev marked this conversation as resolved.
Show resolved Hide resolved
}
27 changes: 21 additions & 6 deletions static/app/gettingStartedDocs/android/android.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import {t, tct} from 'sentry/locale';
// Configuration Start
export const steps = ({
dsn,
}: {
dsn?: string;
} = {}): LayoutProps['steps'] => [
sourcePackageRegistries,
}: Partial<
Pick<ModuleProps, 'dsn' | 'sourcePackageRegistries'>
> = {}): LayoutProps['steps'] => [
{
type: StepType.INSTALL,
description: (
Expand All @@ -28,10 +29,14 @@ export const steps = ({
configurations: [
{
language: 'groovy',
loading: sourcePackageRegistries?.isLoading,
code: `
plugins {
id "com.android.application" // should be in the same module
id "io.sentry.android.gradle" version "3.11.1"
id "io.sentry.android.gradle" version "${
sourcePackageRegistries?.data?.['sentry.java.android.gradle-plugin']?.version ??
priscilawebdev marked this conversation as resolved.
Show resolved Hide resolved
'3.11.1'
}"
}
`,
},
Expand Down Expand Up @@ -139,8 +144,18 @@ export const nextSteps = [
];
// Configuration End

export function GettingStartedWithAndroid({dsn, ...props}: ModuleProps) {
return <Layout steps={steps({dsn})} nextSteps={nextSteps} {...props} />;
export function GettingStartedWithAndroid({
dsn,
sourcePackageRegistries,
...props
}: ModuleProps) {
return (
<Layout
steps={steps({dsn, sourcePackageRegistries})}
nextSteps={nextSteps}
{...props}
/>
);
}

export default GettingStartedWithAndroid;
26 changes: 20 additions & 6 deletions static/app/gettingStartedDocs/apple/apple-ios.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import {t, tct} from 'sentry/locale';
// Configuration Start
export const steps = ({
dsn,
}: {
dsn?: string;
} = {}): LayoutProps['steps'] => [
sourcePackageRegistries,
}: Partial<
Pick<ModuleProps, 'dsn' | 'sourcePackageRegistries'>
> = {}): LayoutProps['steps'] => [
{
type: StepType.INSTALL,
description: (
Expand Down Expand Up @@ -44,8 +45,11 @@ https://github.com/getsentry/sentry-cocoa.git
</p>
),
language: 'swift',
loading: sourcePackageRegistries?.isLoading,
code: `
.package(url: "https://github.com/getsentry/sentry-cocoa", from: "8.9.3"),
.package(url: "https://github.com/getsentry/sentry-cocoa", from: "${
sourcePackageRegistries?.data?.['sentry.cocoa']?.version ?? '8.9.3'
}"),
`,
},
],
Expand Down Expand Up @@ -231,8 +235,18 @@ export const nextSteps = [
];
// Configuration End

export function GettingStartedWithIos({dsn, ...props}: ModuleProps) {
return <Layout steps={steps({dsn})} nextSteps={nextSteps} {...props} />;
export function GettingStartedWithIos({
dsn,
sourcePackageRegistries,
...props
}: ModuleProps) {
return (
<Layout
steps={steps({dsn, sourcePackageRegistries})}
nextSteps={nextSteps}
{...props}
/>
);
}

export default GettingStartedWithIos;
26 changes: 20 additions & 6 deletions static/app/gettingStartedDocs/apple/apple-macos.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import {t, tct} from 'sentry/locale';
// Configuration Start
export const steps = ({
dsn,
}: {
dsn?: string;
} = {}): LayoutProps['steps'] => [
sourcePackageRegistries,
}: Partial<
Pick<ModuleProps, 'dsn' | 'sourcePackageRegistries'>
> = {}): LayoutProps['steps'] => [
Comment on lines +10 to +13
Copy link
Member

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.

Copy link
Member Author

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

{
type: StepType.INSTALL,
description: (
Expand Down Expand Up @@ -44,8 +45,11 @@ https://github.com/getsentry/sentry-cocoa.git
</p>
),
language: 'swift',
loading: sourcePackageRegistries?.isLoading,
code: `
.package(url: "https://github.com/getsentry/sentry-cocoa", from: "8.9.3"),
.package(url: "https://github.com/getsentry/sentry-cocoa", from: "${
sourcePackageRegistries?.data?.['sentry.cocoa']?.version ?? '8.9.3'
}"),
`,
},
],
Expand Down Expand Up @@ -183,8 +187,18 @@ export const nextSteps = [
];
// Configuration End

export function GettingStartedWithMacos({dsn, ...props}: ModuleProps) {
return <Layout steps={steps({dsn})} nextSteps={nextSteps} {...props} />;
export function GettingStartedWithMacos({
dsn,
sourcePackageRegistries,
...props
}: ModuleProps) {
return (
<Layout
steps={steps({dsn, sourcePackageRegistries})}
nextSteps={nextSteps}
{...props}
/>
);
}

export default GettingStartedWithMacos;
26 changes: 20 additions & 6 deletions static/app/gettingStartedDocs/apple/apple.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import {t, tct} from 'sentry/locale';
// Configuration Start
export const steps = ({
dsn,
}: {
dsn?: string;
} = {}): LayoutProps['steps'] => [
sourcePackageRegistries,
}: Partial<
Pick<ModuleProps, 'dsn' | 'sourcePackageRegistries'>
> = {}): LayoutProps['steps'] => [
{
type: StepType.INSTALL,
description: (
Expand Down Expand Up @@ -44,8 +45,11 @@ https://github.com/getsentry/sentry-cocoa.git
</p>
),
language: 'swift',
loading: sourcePackageRegistries?.isLoading,
code: `
.package(url: "https://github.com/getsentry/sentry-cocoa", from: "8.9.3"),
.package(url: "https://github.com/getsentry/sentry-cocoa", from: "${
sourcePackageRegistries?.data?.['sentry.cocoa']?.version ?? '8.9.3'
}"),
`,
},
],
Expand Down Expand Up @@ -183,8 +187,18 @@ export const nextSteps = [
];
// Configuration End

export function GettingStartedWithApple({dsn, ...props}: ModuleProps) {
return <Layout steps={steps({dsn})} nextSteps={nextSteps} {...props} />;
export function GettingStartedWithApple({
dsn,
sourcePackageRegistries,
...props
}: ModuleProps) {
return (
<Layout
steps={steps({dsn, sourcePackageRegistries})}
nextSteps={nextSteps}
{...props}
/>
);
}

export default GettingStartedWithApple;
Loading
Loading