chore: span push and sentry logs #36682
Conversation
WalkthroughThe pull request introduces several modifications across four files. In Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UserSagas
participant Login
participant SignUp
participant Sentry
User->>UserSagas: Create User
UserSagas->>UserSagas: Attempt to create user
alt Error occurs
UserSagas->>Sentry: captureException(error)
end
User->>Login: Attempt to log in
alt Error occurs
Login->>Sentry: captureException(error)
end
User->>SignUp: Attempt to sign up
alt Error occurs
SignUp->>Sentry: captureException(error)
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11164876382. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (2)
14-14: Excellent addition to our imports, students!I see you've imported the AUTHENTICATE constant. This is a good step towards making our code more readable and maintainable. However, let's take this opportunity for a quick lesson:
When we're importing constants, it's often beneficial to import the entire class statically, rather than individual constants. This can make our code more flexible if we need to use other constants from the same class in the future.
Consider changing the import to:
import static com.appsmith.external.constants.spans.BaseSpan.*;This way, we'll have access to all constants in the BaseSpan class without needing to modify our imports again.
51-54: Well done on expanding our span inclusion criteria!I'm impressed with how you've broadened our span filtering to include authentication-related spans. This is a significant improvement that will enhance our tracing capabilities. Let's break it down:
- We're still including server-side spans and those with the APPSMITH_SPAN_PREFIX.
- We've added conditions for spans starting with AUTHENTICATE or "authorize".
This is good work, but let's make it even better with a small suggestion:
Consider extracting the "authorize" string as a constant, similar to AUTHENTICATE. This will improve consistency and make future modifications easier. For example:
private static final String AUTHORIZE_PREFIX = "authorize"; // Then in the method: || finishedSpan.getName().startsWith(AUTHENTICATE) || finishedSpan.getName().startsWith(AUTHORIZE_PREFIX)Remember, constants make our code more maintainable and less prone to typos. Keep up the excellent work, class!
app/client/src/ce/sagas/userSagas.tsx (1)
133-138: Excellent error handling, A+ work!You've shown great initiative in capturing exceptions with Sentry. This will help us keep a close eye on any sign-up troubles. Remember, class, always be kind to your users and help them when they stumble!
However, let's make sure we're using our vocabulary correctly. Instead of "Sign up failed", let's be more specific:
Consider changing the error message to be more descriptive:
- Sentry.captureException("Sign up failed", { + Sentry.captureException("User creation failed", { level: Severity.Error, extra: { error: error, }, });This way, we're being precise about what exactly failed. Precision is key in programming, just like in spelling tests!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/ce/sagas/userSagas.tsx (2 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/BaseSpan.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (2 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (1)
8-8: Very good, class! Let's add some logging capabilities.I'm pleased to see you've imported the Slf4j logger. This will allow us to add informative log messages to our code, which is always a smart practice. Keep up the good work!
app/client/src/ce/sagas/userSagas.tsx (2)
91-92: Good job on importing Sentry, class!I see you've added Sentry to our toolkit. That's a gold star for improved error tracking! Remember, children, always keep an eye on those pesky errors.
Line range hint
1-1006: Overall assessment: Great progress, keep up the good work!Class, today we've learned an important lesson about error handling. These changes will help us better understand when our users have trouble signing up. Remember, in the world of programming, being able to see and understand our mistakes is just as important as getting things right the first time. Keep up the excellent work, and don't forget to always be learning!
...server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/BaseSpan.java
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-36682.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11166073058. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/src/pages/UserAuth/SignUp.tsx (2)
138-143: Excellent work on error handling, but let's make it even better!You've done a splendid job integrating Sentry for error tracking. It's like giving our code a fancy new pair of glasses to spot troubles! However, we can make this even more helpful for our users.
Here's a little homework for you:
- Consider adding a user-friendly error message to display on the UI.
- Think about categorizing errors to provide more specific feedback.
- Maybe we could add some logging to help us troubleshoot locally too!
Remember, clear communication is key, both in the classroom and in our code!
Line range hint
1-324: A gold star for your efforts in improving our SignUp process!Class, let's take a moment to appreciate the fine work done here. Our SignUp component has gotten a nice upgrade, like getting a new backpack for the school year!
Here's what I like:
- You've added error tracking without disrupting the main flow. Well done!
- The code is still neat and tidy, just like your desks should be.
For your next assignment, consider these areas for improvement:
- Could we add some comments to explain the Sentry integration for future students?
- Is there a way to make our error messages more friendly, like how we greet each other in the morning?
- Think about how we might test these new changes to make sure they're working perfectly.
Remember, continuous improvement is the key to success, both in coding and in life!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/pages/UserAuth/Login.tsx (2 hunks)
- app/client/src/pages/UserAuth/SignUp.tsx (2 hunks)
🔇 Additional comments (1)
app/client/src/pages/UserAuth/SignUp.tsx (1)
60-61: Very good, class! Let's welcome our new friends from Sentry.I'm pleased to see you've added these imports for Sentry. This will help us keep a watchful eye on any mischief happening in our application. Remember, children, monitoring errors is like having a hall monitor for your code!
| Sentry.captureException("Sign up failed", { | ||
| level: Severity.Error, | ||
| extra: { | ||
| error: new Error(errorMessage), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct Usage of Sentry.captureException
Let's focus on the error handling block in lines 117-122. Currently, "Sign up failed" is passed as a string to Sentry.captureException, but this function expects an Error object. Additionally, setting the level directly in captureException isn't the recommended approach.
Here's how we can refactor the code to use Sentry.withScope for better context and proper severity level:
if (queryParams.get("error")) {
errorMessage = queryParams.get("message") || queryParams.get("error") || "";
showError = true;
- Sentry.captureException("Sign up failed", {
- level: Severity.Error,
- extra: {
- error: new Error(errorMessage),
- },
- });
+ Sentry.withScope(function(scope) {
+ scope.setLevel('error');
+ scope.setExtra('errorMessage', errorMessage);
+ Sentry.captureException(new Error('Sign up failed'));
+ });
}This adjustment ensures we're capturing the exception with the appropriate error object and severity level.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Sentry.captureException("Sign up failed", { | |
| level: Severity.Error, | |
| extra: { | |
| error: new Error(errorMessage), | |
| }, | |
| }); | |
| Sentry.withScope(function(scope) { | |
| scope.setLevel('error'); | |
| scope.setExtra('errorMessage', errorMessage); | |
| Sentry.captureException(new Error('Sign up failed')); | |
| }); |
|
Deploy-Preview-URL: https://ce-36682.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11177807884. |
|
Deploy-Preview-URL: https://ce-36682.dp.appsmith.com |
ApekshaBhosale
changed the title
ApekshaBhosale
changed the title
github-actions
bot
added
the
skip-changelog
| @@ -9,4 +9,8 @@ public class BaseSpan { | |||
| public static final String GIT_SPAN_PREFIX = "git."; | |||
|
|
|||
| public static final String APPLICATION_SPAN_PREFIX = "application."; | |||
|
|
|||
| public static final String AUTHENTICATE = "authenticate"; | |||
There was a problem hiding this comment.
Can we use more specific strings here? Just to make sure we know exactly what we're tracking?
There was a problem hiding this comment.
i have added general because i want to collect all authenticate related spans. tell me if all authenticate spans will be authenticate usernamepassword. this one we get with form login @nidhi-nair
There was a problem hiding this comment.
Okay, may be we'll know more as we gather data. Let's go ahead with this for now.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11182676913
Commit: c93c9dc
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 04 Oct 2024 16:45:18 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation