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(builder): sentry integration #8053

Merged

Conversation

ntindle
Copy link
Member

@ntindle ntindle commented Sep 13, 2024

Background

We want to track errors in the frontend

Changes 🏗️

Adds sentry and configures a few of its more straightforward settings to meet our goals. The main one I changed was the actions to work

(note 90%+ of this was scaffolded by the sentry wizard 🧙 )

PR Quality Scorecard ✨

  • Have you used the PR description template?   +2 pts
  • Is your pull request atomic, focusing on a single change?   +5 pts
  • Have you linked the GitHub issue(s) that this PR addresses?   +5 pts
  • Have you documented your changes clearly and comprehensively?   +5 pts
  • Have you changed or added a feature?   -4 pts
    • Have you added/updated corresponding documentation?   +4 pts
    • Have you added/updated corresponding integration tests?   +5 pts
  • Have you changed the behavior of AutoGPT?   -5 pts
    • Have you also run agbenchmark to verify that these changes do not regress performance?   +10 pts

@ntindle ntindle requested a review from a team as a code owner September 13, 2024 22:31
@ntindle ntindle requested review from Swiftyos and Torantulino and removed request for a team September 13, 2024 22:31
@github-actions github-actions bot added the platform/frontend AutoGPT Platform - Front end label Sep 13, 2024
@ntindle ntindle requested review from Pwuts and majdyz and removed request for Swiftyos and Torantulino September 13, 2024 22:31
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The Sentry DSN is hardcoded in the client-side configuration file (rnd/autogpt_builder/sentry.client.config.ts, line 8). This could potentially expose sensitive information to users. It's recommended to use environment variables for such sensitive data, especially in client-side code.

⚡ Key issues to review

Sensitive Information Exposure
The Sentry DSN is hardcoded in the client-side configuration file, which may expose sensitive information.

Error Handling
The Sentry instrumentation wraps the entire function body without specific error handling, which may lead to unhandled exceptions.

Configuration Complexity
The Sentry configuration in next.config.mjs is extensive and may require careful review to ensure all settings are appropriate for the project's needs.

Copy link

netlify bot commented Sep 13, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit 53cca1f
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/66e844494e30940008624091

@ntindle
Copy link
Member Author

ntindle commented Sep 13, 2024

DSNs are safe to keep public because they only allow submission of new events and related event data; they do not allow read access to any information.

https://docs.sentry.io/concepts/key-terms/dsn-explainer/#dsn-utilization

@ntindle ntindle enabled auto-merge (squash) September 13, 2024 22:35
@Swiftyos
Copy link
Contributor

What does sentry offer that we are not currently getting with google analytics?

@ntindle
Copy link
Member Author

ntindle commented Sep 16, 2024

Backend errors and enables tracing errors through end to end. Also includes things like replays but I'm pretty sure GA offers that. Also handles ad blockers by sending analytics from the backend

@ntindle ntindle merged commit 22ce8e0 into master Sep 16, 2024
12 checks passed
@ntindle ntindle deleted the ntindle/secrt-854-hook-to-frontend-for-user-activitiyevents branch September 16, 2024 16:19
const all = rows.map((row) => removeFeaturedAgent(row.id));
await Promise.all(all);
revalidatePath("/marketplace");
return await Sentry.withServerActionInstrumentation(
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, but I think this is definitely worth a follow-up:
The changes wrapping sentry here can be a wrapper function.

Copy link
Member Author

Choose a reason for hiding this comment

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

100% agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants