fix: Ensure Sentry is initialized for non logged in users too #1183
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| if not user_file.exists(): | ||
| update_dsn() | ||
|
|
There was a problem hiding this comment.
Unnecessary update_dsn() call. The user_data.json file is created in update_dsn if it doesn't exist, so calling update_dsn before checking if the file exists is redundant.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if not user_file.exists(): | |
| update_dsn() | |
| if not user_file.exists(): |
python/composio/utils/sentry.py
Outdated
| @atexit.register | ||
| def update_dsn() -> None: | ||
| user_file = Path.home() / ".composio" / "user_data.json" | ||
| if not user_file.exists(): | ||
| return | ||
|
|
||
| try: | ||
| data = json.loads(user_file.read_text(encoding="utf-8")) | ||
| except json.JSONDecodeError: | ||
| return | ||
|
|
||
| if data.get("api_key") is None: | ||
| return | ||
|
|
||
| if data.get("sentry") is not None and data.get("sentry").get("dsn") is not None: | ||
| if user_file.exists(): | ||
| try: | ||
| data = json.loads(user_file.read_text(encoding="utf-8")) | ||
| except json.JSONDecodeError: | ||
| return | ||
| else: | ||
| data: t.Dict[str, t.Any] = {} | ||
|
|
||
| if data.get("sentry", {}).get("dsn") is not None: |
There was a problem hiding this comment.
The update_dsn function now unnecessarily creates an empty dictionary if the user file doesn't exist, even if the DSN is already available. This could lead to unintended data overwrites if the file is later created.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 24d8ba1 in 47 seconds
More details
- Looked at
58lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. python/composio/utils/sentry.py:26
- Draft comment:
Using@cacheonfetch_dsnmay lead to stale DSN values if they change on the server. Consider removing the cache if DSN updates are expected. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The DSN is only fetched in very specific circumstances: 1) when the program exits and there's no DSN stored, or 2) when the user file doesn't exist. Once fetched, it's stored in a file. The cache doesn't matter because the value is persisted to disk and subsequent runs will use the file value. Within a single run, caching is fine because we don't need real-time DSN updates.
Maybe there are scenarios where the program needs to refresh the DSN mid-execution that I'm not seeing. The caching could prevent that.
Looking at the code flow, the DSN is only used for initialization and stored persistently. There's no mechanism or apparent need for mid-execution DSN updates.
The comment should be deleted because the caching behavior is appropriate for this use case. The DSN is meant to be stable within a program execution and is properly persisted between runs.
2. python/composio/utils/sentry.py:111
- Draft comment:
Ensure the.composiodirectory exists before writing touser_data.jsonto avoidFileNotFoundError. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_YGIbAIKfqlkBwQFT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -34,6 +36,9 @@ def fetch_dsn() -> t.Optional[str]: | |||
|
|
|||
| def get_sentry_config() -> t.Optional[t.Dict]: | |||
There was a problem hiding this comment.
The get_sentry_config() function's return type hint t.Optional[t.Dict] should be more specific. Consider using t.Optional[t.Dict[str, t.Any]] to better document the expected structure of the configuration dictionary.
| @@ -106,18 +111,15 @@ def init(): | |||
| @atexit.register | |||
| def update_dsn() -> None: | |||
There was a problem hiding this comment.
Consider adding a docstring to the update_dsn() function explaining its purpose, behavior, and the fact that it's registered as an atexit handler. This would help future maintainers understand when and how this function is called.
| @@ -22,6 +23,7 @@ | |||
| import sentry_sdk.types | |||
|
|
|||
|
|
|||
| @cache | |||
| def fetch_dsn() -> t.Optional[str]: | |||
There was a problem hiding this comment.
The fetch_dsn() function is now cached using @cache decorator. Consider adding a comment or updating the docstring to indicate this caching behavior, as it affects the function's performance characteristics and potential memory usage.
Code Review SummaryOverall, this is a well-structured PR that improves Sentry initialization for non-logged-in users. Here's my assessment: Strengths:✅ Addition of caching for Suggestions for Improvement:
Code Quality Rating: 8/10The changes are solid, improve system reliability, and fix important edge cases. The code is well-structured and maintainable. |
| @@ -34,6 +36,9 @@ def fetch_dsn() -> t.Optional[str]: | |||
|
|
|||
There was a problem hiding this comment.
The @cache decorator on fetch_dsn could lead to stale DSNs being used. If the DSN changes on the backend, the cached value will prevent the updated DSN from being retrieved until the process restarts.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @cache_with_ttl(ttl_seconds=300) | |
| def fetch_dsn() -> t.Optional[str]: |
python/composio/utils/sentry.py
Outdated
| if user_file.exists(): | ||
| try: | ||
| data = json.loads(user_file.read_text(encoding="utf-8")) | ||
| except json.JSONDecodeError: | ||
| return | ||
| else: | ||
| data: t.Dict[str, t.Any] = {} | ||
|
|
||
| if data.get("sentry", {}).get("dsn") is not None: | ||
| return | ||
|
|
||
| dsn = fetch_dsn() | ||
| if dsn is None: | ||
| return |
There was a problem hiding this comment.
The logic for updating the DSN is unnecessarily complex and can be simplified. The nested if statements and redundant checks can be removed.
| if not user_file.exists(): | ||
| update_dsn() | ||
|
|
There was a problem hiding this comment.
Calling update_dsn() when the user file doesn't exist in get_sentry_config() might lead to unnecessary network requests. This function's primary purpose seems to be configuration retrieval, not updating.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if not user_file.exists(): | |
| update_dsn() | |
| if not user_file.exists(): | |
| return None |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 46fe3c8 in 25 seconds
More details
- Looked at
10lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/utils/sentry.py:130
- Draft comment:
Consider moving the directory creation lineuser_file.parent.mkdir(parents=True, exist_ok=True)before any file operations to ensure the directory exists before attempting to read or write the file. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_wpOZJjoiGL7jV0sL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -22,6 +23,7 @@ | |||
| import sentry_sdk.types | |||
|
|
|||
|
|
|||
| @cache | |||
There was a problem hiding this comment.
The @cache decorator on fetch_dsn could lead to stale DSNs being used. If the DSN changes on the backend, the client will not pick up the change until the process restarts.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @cache | |
| @cache(ttl=300) |
| if not user_file.exists(): | ||
| update_dsn() | ||
|
|
There was a problem hiding this comment.
Unnecessary call to update_dsn() if the user file doesn't exist. The subsequent if not user_file.exists(): return None already handles this case.
| return | ||
|
|
||
| dsn = fetch_dsn() | ||
| if dsn is None: | ||
| return | ||
|
|
||
| data["sentry"] = {"dsn": dsn} | ||
| user_file.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
The logic in update_dsn to create the user data directory is now performed even if the DSN is already present, which is unnecessary.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| user_file.parent.mkdir(parents=True, exist_ok=True) | |
| if not user_file.exists(): | |
| user_file.parent.mkdir(parents=True, exist_ok=True) |
| import traceback | ||
| import types | ||
| import typing as t | ||
| from functools import cache |
There was a problem hiding this comment.
Re-importing 'cache' from functools can lead to confusion and potential import errors. Remove the duplicate import statement.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2621fee in 11 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/utils/sentry.py:9
- Draft comment:
Unnecessary change in import order. Consider reverting to maintain consistency. - Reason this comment was not posted:
Confidence changes required:10%
The import statement for 'cache' from 'functools' was moved, but this change is unnecessary and doesn't affect functionality. It should be reverted for consistency.
Workflow ID: wflow_jUN2eodN0D11ExX6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| except json.JSONDecodeError: | ||
| return | ||
| else: | ||
| data = {} |
There was a problem hiding this comment.
Removing the type hint reduces code maintainability and can lead to type-related bugs. Reintroduce the type hint for clarity and error prevention. The type hint for 'data' was removed, which may lead to type-related issues if 'data' is used inconsistently elsewhere.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 01a5d91 in 14 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/composio/utils/sentry.py:120
- Draft comment:
Consider keeping the type hint for 'data' for clarity and type safety.
data: t.Dict[str, t.Any] = {}
- Reason this comment was not posted:
Confidence changes required:50%
The type hint for 'data' was removed, which can lead to less clarity and potential type-related issues in the future.
Workflow ID: wflow_UXk53iTAdwAONPWg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Ensure Sentry is initialized for users without
user_data.jsonby updatingget_sentry_config()and cachingfetch_dsn().user_data.jsonby callingupdate_dsn()inget_sentry_config().update_dsn()now creates directory structure ifuser_data.jsonis absent.fetch_dsn()to optimize repeated calls.This description was created by
for 01a5d91. It will automatically update as commits are pushed.