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

feat: Rework Browser JS integration docs #7648

Merged
merged 18 commits into from
Aug 28, 2023
Merged

feat: Rework Browser JS integration docs #7648

merged 18 commits into from
Aug 28, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Aug 16, 2023

This PR rewrites & reorganizes the integration docs for Browser JS SDKs.

Now, there is a single Integrations page that lists all integrations available for this SDK, with a short description & a note if it is enabled by default.

Each integration has it's own detail page with eventually available options.

Screenshot 2023-08-16 at 12 32 42

The one special case is Electron, which does not have the same default integrations. I found a way to hide the parent pages for Electron, maybe a bit hacky (??) but it seems to work fine... 🤷

Screenshot 2023-08-16 at 12 42 08

Another change I had to make to make this work/make sense is to move React/Vue Router & Redux pages, which have been inside of Integrations, into the React/Vue Features space. IMHO that makes more sense anyhow, as it is not really integrations this is about...?

Note that this does not touch Node.js yet, we can do that in a follow up.

@mydea mydea requested review from a team August 16, 2023 10:52
@mydea mydea self-assigned this Aug 16, 2023
@mydea mydea requested review from Lms24 and AbhiPrasad and removed request for a team August 16, 2023 10:52
@vercel
Copy link

vercel bot commented Aug 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sentry-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 24, 2023 0:01am

@mydea
Copy link
Member Author

mydea commented Aug 16, 2023

Note: We can/should also improve this page a bit, maybe: https://sentry-docs-git-fn-js-integrations.sentry.dev/platforms/javascript/configuration/integrations/custom/

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

I like this change!! The individual pages make a lot more sense to me, and hopefully is easier to maintain as well

Copy link
Contributor

@lizokm lizokm left a comment

Choose a reason for hiding this comment

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

Still working through this :)

@@ -99,6 +99,6 @@ As a best practice you should always avoid logging confidential information. If

- Anonymize the confidential information within the log statements (for example, swap out email addresses -> for internal identifiers)
- Use <PlatformIdentifier name="before-breadcrumb" /> to filter it out from breadcrumbs before it is attached
- Disable logging breadcrumb integration (for example, as described [here](/platforms/javascript/configuration/integrations/default/#breadcrumbs))
- Disable logging breadcrumb integration (for example, as described [here](/platforms/javascript/configuration/integrations/breadcrumbs/))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Disable logging breadcrumb integration (for example, as described [here](/platforms/javascript/configuration/integrations/breadcrumbs/))
- Disable the breadcrumb logging integration (read more [here](/platforms/javascript/configuration/integrations/breadcrumbs/))

There isn't anything on the Breadcrumbs page we're linking to that specifically says "how to disable the breadcrumb logging integration" - is it possible to write brief instructions for how to do it here and save our users an extra click? Is it as easy as disabling the Sentry.Integrations.Breadcrumbs integration?

Copy link
Member Author

Choose a reason for hiding this comment

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

well then you disable all breadcrumbs, you're gonna want to do this:

integrations: [
  new Sentry.Integrations.Breadcrumbs({
    console: false
  })
]

to only disable console breadcrumbs. But based on this, let's add a sentence to the breadcrumbs integration page explicitly pointing out that the options below are meant to disable specific breadcrumbs, and why you may want to do this 👍


This integration deduplicates certain events. It can be helpful if you're receiving many duplicate errors. Note, that Sentry only compares stack traces and fingerprints.

<PlatformContent includePath="configuration/dedupe" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<PlatformContent includePath="configuration/dedupe" />
<PlatformContent includePath="configuration/dedupe" />

Screenshot 2023-08-17 at 3 59 43 PM

Heads up that the "logged in" link in the above note takes you to:

Screenshot 2023-08-17 at 4 01 02 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah that doesn't work in the preview! sentry always redirects back to docs.sentry.io and there the page does not exist 😅 (same for all of these)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, that makes sense!

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

I like this change! Makes the structure much more clear and easier to navigate between individual integrations! I have a few high-level comments. Feel free to apply/disregard as you see fit. We can also tackle them as a follow-up.

  • m: I'd like to see on first glance which integrations are enabled by default and which need to be added by users. Maybe we can put this directly under the title of the individual pages? Given that we're no longer distinguishing between default and pluggable integrations I think this is important. wdyt?
  • l: Great that we now have a dedicated Options section! More a cosmetic thing than anything but can we give the individual options a heading level? This way we can link to specific options and I guess it'd look a little better than the bullet points
  • l: Totally optional but wdyt about adding BrowserTracing and Replay to the list? We shouldn't duplicate information there but just link to the dedicated performance/replay docs.

@Lms24
Copy link
Member

Lms24 commented Aug 18, 2023

Oh btw, great decision to move the Vue/React router stuff into the "[Framework] Features" section - I like it!

@mydea
Copy link
Member Author

mydea commented Aug 18, 2023

OK, I updated this based on feedback by @Lms24:

  1. Added a note at the top of each default integration stating that it is default, + linking to the section telling you how to customize default integrations.
  2. Refactored all options to be headings, to make it easier to link there etc.
  3. Added a page for Replay and BrowserTracing integrations, which bascially link to performance/replay setup for more details.
  4. Added the ContextLines integration which I forgot to add before 😅
  5. I actually figured out there is a much nicer way to filter out integrations for electron, we can just provide notSupported to the page and it will be gone 🤦
  6. Improved the Custom Integrations page a bit with an actual example for a custom integration

description: "Capture performance data for the Browser."
sidebar_order: 99
notSupported:
- javascript.cordova
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, not sure why cordova does not have a performance section? 🤔

description: "Capture a video-like reproduction of what was happening in the user's browser."
sidebar_order: 99
notSupported:
- javascript.cordova
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if there is a reason cordova does not have a replay section?

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks for applying my feedback!

Copy link
Contributor

@lizokm lizokm left a comment

Choose a reason for hiding this comment

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

Phew!! Thanks for this amazing, labor-intensive PR!


This integration deduplicates certain events. It can be helpful if you're receiving many duplicate errors. Note, that Sentry only compares stack traces and fingerprints.

<PlatformContent includePath="configuration/dedupe" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, that makes sense!

src/platforms/react-native/troubleshooting.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@shanamatthews shanamatthews left a comment

Choose a reason for hiding this comment

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

Looks good, just a few questions from me here

Comment on lines 12 to 14
## Available Integrations

<PageGrid exclude={["custom"]} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we might want to move this below the instructions for adding/removing/custom integrations. That info is important and is very easy to miss with this long list of integrations folks won't scroll through.

These integrations are listed in the sidebar too.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 sure, I can move it down!

description: "Adds app, operating system and runtime context to all events. (default)"
sidebar_order: 10
redirect_from:
- /platforms/javascript/guides/electron/configuration/integrations/default/
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a better redirect to /platforms/javascript/guides/electron/configuration/integrations/ using vercel.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, will replace this with a proper redirect for all!

Comment on lines +62 to +65
const aBase = a.context.sidebar_order ?? 10;
const bBase = b.context.sidebar_order ?? 10;
const aso = aBase >= 0 ? aBase : 10;
const bso = bBase >= 0 ? bBase : 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, forgot to mention this: I noticed that this was actually buggy before, because sidebar_order was sometimes null which lead to it not getting the default of 10, but null, leading to incorrect ordering etc.

@mydea mydea force-pushed the fn/js-integrations branch from 67c63ce to 44f8e00 Compare August 24, 2023 11:43
@mydea mydea closed this Aug 28, 2023
@mydea mydea reopened this Aug 28, 2023
@mydea mydea merged commit 1b46a11 into master Aug 28, 2023
@mydea mydea deleted the fn/js-integrations branch August 28, 2023 13:37
@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants