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(astro): Update getting started page #11953

Merged
merged 9 commits into from
Nov 28, 2024
Merged

Conversation

chargome
Copy link
Member

documents getsentry/sentry-javascript#14274

  • Documents manual configuration files as default
  • Adds onboarding options for replay, tracing and profiling
  • Removes manual setup subpage
  • Adds a redirect from manual setup to getting started to avoid dead links from external pages

@chargome chargome self-assigned this Nov 26, 2024
Copy link

vercel bot commented Nov 26, 2024

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 Nov 28, 2024 9:10am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
changelog ⬜️ Ignored (Inspect) Visit Preview Nov 28, 2024 9:10am
develop-docs ⬜️ Ignored (Inspect) Visit Preview Nov 28, 2024 9:10am

Copy link

codecov bot commented Nov 26, 2024

Bundle Report

Changes will increase total bundle size by 15 bytes (0.0%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
sentry-docs-server-cjs 10.13MB 21 bytes (0.0%) ⬆️
sentry-docs-client-array-push 9.02MB 6 bytes (-0.0%) ⬇️

Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Generally works for me! I'll let @Lms24 also give it a pass though.

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 taking this on, Charly! General thoughts:

  • I'm okay with removing manual setup and merging it with Getting Started as long as we don't have a wizard. Which in all likelihood we won't have for Astro.
  • I'd like us to continue using Astro's CLI to add the Astro integration because it's just what users are used to do. Unless there's a good reason not to. Definitely open to hearing your thoughts on this though!

Copy link
Member

Choose a reason for hiding this comment

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

m/h: What is our rationale behind moving away from npx astro add @sentry/astro? Despite us instructing people to declare separate client|server config files, the astro CLI would still automatically register the intgration in astro.config.mjs for users.

Users are probably familiar with adding Astro integrations and we're also listed in their official integrations directory: https://astro.build/integrations/?search=sentry

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, I'll update that

Copy link
Member Author

Choose a reason for hiding this comment

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

the only thing that bugged me here is that we have another code snippet for adding profiling

Copy link
Member

@Lms24 Lms24 Nov 26, 2024

Choose a reason for hiding this comment

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

l/no action required: I find it a bit unfortunate that the advanced config section is also on the Getting Started page. We might want to consider moving this somewhere else. Previously, "Manual Setup" gave us the option to write all of this down but still have a short and concise "Getting Started" page. I think this might be worth to revisit later, potentially after the rework on the "Getting Started" pages (contrator?) of other SDKs is in a good state 🤔

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 was not sure where to put this tbh, I guess just adding an advanced config sub page also would not make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Lms24 merging this so we get this out for now, we can do a follow up!

Copy link
Member

Choose a reason for hiding this comment

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

sorry, for not reviewing this in time! I'm fine with leaving it as-is for the moment

});
```

### Server-side Setup
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
### Server-side Setup
### Server-Side Setup

Copy link
Contributor

@coolguyzone coolguyzone 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 the updates! Added a couple suggestions, looks good to me! 🫡🦃

@chargome chargome merged commit 09bdfb3 into master Nov 28, 2024
13 checks passed
@chargome chargome deleted the cg/astro-runtime-init branch November 28, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants