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

[docs] SDK 50 guide for Sentry #25712

Merged
merged 10 commits into from
Jan 18, 2024
Merged

[docs] SDK 50 guide for Sentry #25712

merged 10 commits into from
Jan 18, 2024

Conversation

brentvatne
Copy link
Member

@brentvatne brentvatne commented Dec 2, 2023

Why

Ensure we have instructions for using @sentry/react-native with SDK 50.

TODO

  • Test the latest release after speaking with the Sentry team on Jan 10
  • Update this page to indicate that sentry-expo is deprecated in SDK 50

@expo-bot expo-bot added the bot: passed checks ExpoBot has nothing to complain about label Dec 2, 2023
@expo-bot
Copy link
Collaborator

expo-bot commented Dec 2, 2023

📘 Your docs preview website is ready!

Comment on lines 415 to 419
<ConfigReactNative>

If you do not use [Continuous Native Generation (CNG)](https://docs.expo.dev/workflow/continuous-native-generation/), then you should use the [@sentry/wizard](https://docs.sentry.io/platforms/react-native/#install).

</ConfigReactNative>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is true. Users will be able to use it to set up apps with CNG.

After getsentry/sentry-wizard#505

Comment on lines 397 to 407
```json app.json
{
"expo": {
"plugins": ["sentry-expo", {
"organization": "sentry org slug, or use the `SENTRY_ORG` environment variable",
"project": "sentry project name, or use the `SENTRY_PROJECT` environment variable"
}]
/* @hide ... your existing configuration */ /* @end */
}
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Should be nested array as per https://docs.expo.dev/config-plugins/introduction/#use-a-config-plugin

Suggested change
```json app.json
{
"expo": {
"plugins": ["sentry-expo", {
"organization": "sentry org slug, or use the `SENTRY_ORG` environment variable",
"project": "sentry project name, or use the `SENTRY_PROJECT` environment variable"
}]
/* @hide ... your existing configuration */ /* @end */
}
}
```
```json app.json
{
"expo": {
"plugins": [["@sentry/react-native", {
"organization": "sentry org slug, or use the `SENTRY_ORG` environment variable",
"project": "sentry project name, or use the `SENTRY_PROJECT` environment variable"
}]
/* @hide ... your existing configuration */ /* @end */
]
}
}

@quinlanj quinlanj self-requested a review December 8, 2023 18:17
docs/pages/guides/using-sentry.mdx Outdated Show resolved Hide resolved

If you are using EAS Build, you can set the `SENTRY_AUTH_TOKEN` environment variable by [creating a secret of the same name](/build-reference/variables/#using-secrets-in-environment-variables).

> **warning** Never commit secrets to your repository. Keep the Sentry auth token in a safe place, outside your repository.
Copy link
Member

Choose a reason for hiding this comment

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

I feel, we should provide more context on what that safe place can be, may be can mention one or two examples here?

docs/pages/guides/using-sentry.mdx Outdated Show resolved Hide resolved
docs/pages/guides/using-sentry.mdx Outdated Show resolved Hide resolved
docs/pages/guides/using-sentry.mdx Outdated Show resolved Hide resolved
docs/pages/guides/using-sentry.mdx Outdated Show resolved Hide resolved
brentvatne and others added 4 commits January 9, 2024 13:54
Co-authored-by: Aman Mittal <amandeepmittal@live.com>
Co-authored-by: Bartosz Kaszubowski <gosimek@gmail.com>
Co-authored-by: Kryštof Woldřich <31292499+krystofwoldrich@users.noreply.github.com>
Co-authored-by: Kryštof Woldřich <31292499+krystofwoldrich@users.noreply.github.com>
@brentvatne brentvatne changed the title [docs] First pass at SDK 50 beta guide for Sentry [docs] SDK 50 guide for Sentry Jan 9, 2024
# Why

Add EAS Update instructions for Sentry integration in SDK 50


# Test Plan

Inspected manually with `yarn run dev`
<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
Comment on lines 203 to 209
### Upload source maps

After running `eas update`, upload the source maps to Sentry:

```sh
SENTRY_AUTH_TOKEN=[super-secret-token] node node_modules/@sentry/react-native/scripts/expo-upload-sourcemaps.js dist
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move this above the Tag configuration section? I know it logically makes sense to first set tags, but I believe uploaded source maps are more important than the tags and I don't want users to overlook this.

```js
import * as Updates from 'expo-updates';

const { metadata, extra } = Updates.manifest;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, I was trying this out in sdk 50 beta, and was getting some type errors for this metadata stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

How about this: #25712 (comment)

Open to suggestions for this suggestion 😄

Copy link
Member

Choose a reason for hiding this comment

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

Should be able to disambiguate the subtype by checking for the presence of metadata: https://github.com/expo/expo/blob/main/apps/expo-go/src/menu/DevMenuTaskInfo.tsx#L28

Something like:

const { metadata, extra } =
    'metadata' in manifest ? manifest : { metadata: undefined, extra: undefined };

or this might be cleaner:

const metadata = 'metadata' in manifest ? manifest.metadata : undefined;
const extra = 'metadata' in manifest ? manifest.extra : undefined;

metadata in the manifest has type object and extra has type ManifestExtra.

Copy link
Member

Choose a reason for hiding this comment

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

based on will's feedback: #25712 (comment)

In this snippet, updateGroup is typed as unknown, so we do a runtime check for 'string' where necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

would this also preserve types?

const { manifest } = Updates;
const metadata = manifest?.metadata;
const extra = metadata ? manifest.extra : undefined;
const updateGroup = metadata?.updateGroup;

Copy link
Collaborator

@Simek Simek Jan 13, 2024

Choose a reason for hiding this comment

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

Can't we use, or cast to the exported Manifest type?

We should be able to discern which exact type it is, by checking the presence of i.e. metadata key, via in operator before casting, if needed, as Will suggested:

const typedManifest = 'metadata' in manifest ? (manifest as ExpoUpdatesManifest) : (manifest as EmbeddedManifest); 

Copy link
Member Author

Choose a reason for hiding this comment

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

this seems like a good suggestion @Simek. up to @quinlanj / @wschurman

Copy link
Member

Choose a reason for hiding this comment

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

It's also possible that it is empty object, though it's somewhat hard to tell when that's the case. Looks like it's when the library is disabled, either explicitly or due to an initialization error. So the actual type of Updates.manifest is ExpoUpdatesManifest | EmbeddedManifest | {} (I'll file a task to change it to not use Partial<Manifest>).

brentvatne and others added 2 commits January 10, 2024 18:51
Co-authored-by: Kryštof Woldřich <31292499+krystofwoldrich@users.noreply.github.com>
@fobos531
Copy link
Contributor

@brentvatne Sidenote, is Sentry yet to publish an updated version of their SDK which includes support for SDK 50? I tried migrating away from sentry-expo in one of my side projects using the instructions in this PR, and it couldn't find the config plugin.

@brentvatne
Copy link
Member Author

@brentvatne Sidenote, is Sentry yet to publish an updated version of their SDK which includes support for SDK 50? I tried migrating away from sentry-expo in one of my side projects using the instructions in this PR, and it couldn't find the config plugin.

hello! yes! give this version a try: "@sentry/react-native": "5.16.0-alpha.4" - it will work with the info in this guide. a stable release with these changes is coming very soon

Comment on lines 195 to 215
```js
import * as Updates from 'expo-updates';

const { metadata, extra } = Updates.manifest;

Sentry.init({
// dsn, release, dist, etc...
});

Sentry.configureScope(scope => {
scope.setTag('expo-update-group-id', metadata?.updateGroup);
scope.setTag('expo-update-id', Updates.updateId);
// This will be `true` if the update is the one embedded in the build, and not one downloaded from the updates server.
scope.setTag('expo-is-embedded-update', Updates.isEmbeddedLaunch);
scope.setTag(
'expo-update-debug-url',
Updates.isEmbeddedLaunch
? 'not applicable for embedded updates'
: `https://expo.dev/accounts/${extra?.expoClient?.owner}/projects/${extra?.expoClient?.slug}/updates/${metadata?.updateGroup}`
);
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```js
import * as Updates from 'expo-updates';
const { metadata, extra } = Updates.manifest;
Sentry.init({
// dsn, release, dist, etc...
});
Sentry.configureScope(scope => {
scope.setTag('expo-update-group-id', metadata?.updateGroup);
scope.setTag('expo-update-id', Updates.updateId);
// This will be `true` if the update is the one embedded in the build, and not one downloaded from the updates server.
scope.setTag('expo-is-embedded-update', Updates.isEmbeddedLaunch);
scope.setTag(
'expo-update-debug-url',
Updates.isEmbeddedLaunch
? 'not applicable for embedded updates'
: `https://expo.dev/accounts/${extra?.expoClient?.owner}/projects/${extra?.expoClient?.slug}/updates/${metadata?.updateGroup}`
);
});
```js
import * as Updates from 'expo-updates';
const manifest = Updates.manifest;
const metadata = "metadata" in manifest ? manifest.metadata : undefined;
const extra = "metadata" in manifest ? manifest.extra : undefined;
const updateGroup =
metadata && "updateGroup" in metadata ? metadata.updateGroup : undefined;
Sentry.init({
// dsn, release, dist, etc...
});
Sentry.configureScope((scope) => {
if (typeof updateGroup === "string") {
scope.setTag("expo-update-group-id", updateGroup);
}
scope.setTag("expo-update-id", Updates.updateId);
// This will be `true` if the update is the one embedded in the build, and not one downloaded from the updates server.
scope.setTag("expo-is-embedded-update", Updates.isEmbeddedLaunch);
scope.setTag(
"expo-update-debug-url",
Updates.isEmbeddedLaunch
? "not applicable for embedded updates"
: `https://expo.dev/accounts/${extra?.expoClient?.owner}/projects/${extra?.expoClient?.slug}/updates/${updateGroup}`
);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

${extra?.expoClient?.owner}

will this possibly lead to /accounts/undefined ever? or is this owner field always actually present and this is just a typing quirk?

/updates/${updateGroup}

it looks like updateGroup can be undefined, so this would lead to /updates/undefined if so

Copy link
Member

@quinlanj quinlanj Jan 15, 2024

Choose a reason for hiding this comment

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

Amended suggestion: #25712 (comment)

on the owner field -- the docs say that this should fall back to username but I tested this and it is indeed undefined. I've made a change where it falls back to [account], and the website should handle this case. The edge case is if slug is undefined but owner isnt (website doesnt handle that). But most manifests have slug so that field should be relatively safe.

@quinlanj quinlanj self-requested a review January 15, 2024 21:06
Comment on lines 196 to 215
import * as Updates from 'expo-updates';

const { metadata, extra } = Updates.manifest;

Sentry.init({
// dsn, release, dist, etc...
});

Sentry.configureScope(scope => {
scope.setTag('expo-update-group-id', metadata?.updateGroup);
scope.setTag('expo-update-id', Updates.updateId);
// This will be `true` if the update is the one embedded in the build, and not one downloaded from the updates server.
scope.setTag('expo-is-embedded-update', Updates.isEmbeddedLaunch);
scope.setTag(
'expo-update-debug-url',
Updates.isEmbeddedLaunch
? 'not applicable for embedded updates'
: `https://expo.dev/accounts/${extra?.expoClient?.owner}/projects/${extra?.expoClient?.slug}/updates/${metadata?.updateGroup}`
);
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import * as Updates from 'expo-updates';
const { metadata, extra } = Updates.manifest;
Sentry.init({
// dsn, release, dist, etc...
});
Sentry.configureScope(scope => {
scope.setTag('expo-update-group-id', metadata?.updateGroup);
scope.setTag('expo-update-id', Updates.updateId);
// This will be `true` if the update is the one embedded in the build, and not one downloaded from the updates server.
scope.setTag('expo-is-embedded-update', Updates.isEmbeddedLaunch);
scope.setTag(
'expo-update-debug-url',
Updates.isEmbeddedLaunch
? 'not applicable for embedded updates'
: `https://expo.dev/accounts/${extra?.expoClient?.owner}/projects/${extra?.expoClient?.slug}/updates/${metadata?.updateGroup}`
);
});
import * as Updates from 'expo-updates';
const manifest = Updates.manifest;
const metadata = "metadata" in manifest ? manifest.metadata : undefined;
const extra = "metadata" in manifest ? manifest.extra : undefined;
const updateGroup =
metadata && "updateGroup" in metadata ? metadata.updateGroup : undefined;
Sentry.init({
// dsn, release, dist, etc...
});
Sentry.configureScope((scope) => {
if (typeof updateGroup === "string") {
scope.setTag("expo-update-group-id", updateGroup);
}
scope.setTag("expo-update-id", Updates.updateId);
// This will be `true` if the update is the one embedded in the build, and not one downloaded from the updates server.
scope.setTag("expo-is-embedded-update", Updates.isEmbeddedLaunch);
if (Updates.isEmbeddedLaunch) {
scope.setTag(
"expo-update-debug-url",
"not applicable for embedded updates"
);
} else if (typeof updateGroup === "string") {
const owner = extra?.expoClient?.owner ?? "[account]";
const slug = extra?.expoClient?.slug ?? "[project]";
scope.setTag(
"expo-update-debug-url",
`https://expo.dev/accounts/${owner}/projects/${slug}/updates/${updateGroup}`
);
}
});

Copy link
Member Author

Choose a reason for hiding this comment

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

@quinlanj - what do you think about this for the configureScope section instead?

Sentry.configureScope((scope) => {
  scope.setTag("expo-update-id", Updates.updateId);
  scope.setTag("expo-is-embedded-update", Updates.isEmbeddedLaunch);

  if (typeof updateGroup === "string") {
    scope.setTag("expo-update-group-id", updateGroup);

    const owner = extra?.expoClient?.owner ?? "[account]";
    const slug = extra?.expoClient?.slug ?? "[project]";
    scope.setTag(
      "expo-update-debug-url",
      `https://expo.dev/accounts/${owner}/projects/${slug}/updates/${updateGroup}`
    );
  } else if (Updates.isEmbeddedLaunch) {
    // This will be `true` if the update is the one embedded in the build, and not one downloaded from the updates server.
    if (Updates.isEmbeddedLaunch) {
      scope.setTag(
        "expo-update-debug-url",
        "not applicable for embedded updates"
      );
    }
  }
});

Copy link
Member

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

Choose a reason for hiding this comment

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

that configureScope suggestion looks good. can remove the second nested Updates.isEmbeddedLaunch check in the else if.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops thanks

@brentvatne brentvatne merged commit bd2af61 into main Jan 18, 2024
5 checks passed
@brentvatne brentvatne deleted the @brent/sentry-sdk-50-guide branch January 18, 2024 18:18
@Simek Simek mentioned this pull request Jan 20, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants