Skip to content

Sync #12

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.

Sign up for GitHub

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

Open
bar-qodo wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Sync #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/sentry/api/serializers/models/group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ def __seen_stats_impl(

return {
item: {
"times_seen": times_seen.get(item.id, 0),
"times_seen": max(times_seen.get(item.id, 0), user_counts.get(item.id, 0)),
Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025 •
edited by AdamWalkerQodo
Loading

Choose a reason for hiding this comment

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

Action Required

4. Times Seen Logic May Hide Issues □ 📎 Requirements Gap

The change modifies times_seen calculation from times_seen.get(item.id, 0) to max(times_seen.get(item.id, 0), user_counts.get(item.id, 0)). This ensures times_seen is never less than user_count, which makes logical sense (you can't have more unique users than total events). However, this change masks a potential data inconsistency issue. If user_counts exceeds times_seen, it indicates a bug in the data collection or aggregation logic that should be investigated and fixed at the source, rather than papered over with a max() call. The fix treats the symptom rather than the root cause.

Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
The max() call ensures times_seen is never less than user_count, which is logically correct. However, if this condition occurs in the data, it indicates a bug in event/user counting that should be investigated.

## Issue Context
The times_seen value comes from tagstore queries, while user_counts comes from a separate query. If user_counts > times_seen, it suggests data inconsistency in how events or users are being tracked.

## Fix Focus Areas
- src/sentry/api/serializers/models/group.py[869-869]

Consider adding:
```python
if user_counts.get(item.id, 0) > times_seen.get(item.id, 0):
    logger.warning(
        "user_count exceeds times_seen",
        extra={"item_id": item.id, "user_count": user_counts.get(item.id, 0), "times_seen": times_seen.get(item.id, 0)}
    )
```

"first_seen": first_seen.get(item.id),
"last_seen": last_seen.get(item.id),
"user_count": user_counts.get(item.id, 0),
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/event_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ def _pull_out_data(jobs: Sequence[Job], projects: ProjectsMapping) -> None:
set_tag(data, "transaction", transaction_name)

job["received_timestamp"] = job["event"].data.get("received") or float(
job["event"].datetime.strftime("%s")
job["event"].datetime.strftime("%s") if job["event"].datetime else 0
Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025 •
edited by AdamWalkerQodo
Loading

Choose a reason for hiding this comment

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

Action Required

3.Potential Type Error with Datetime □ 🐞 Bug

The change adds a check if job["event"].datetime else 0 to handle the case where datetime might be None. However, the Event.datetime property is defined as returning datetime (not Optional[datetime]), and the implementation at line 112-120 shows it will either return a datetime object or raise an exception (e.g., KeyError if 'timestamp' is missing from data). The property cannot return None. If datetime is missing or invalid, the code will raise an exception before reaching the conditional check, making the None check ineffective. Additionally, returning 0 as a fallback creates a type inconsistency - strftime returns a string, but 0 is an integer.

Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
A None check was added for `job["event"].datetime`, but the datetime property cannot return None - it will raise an exception if the timestamp is missing. The check provides false confidence and doesn't actually handle the error case.

## Issue Context
The Event.datetime property is defined to return `datetime` type (not Optional[datetime]). If the timestamp is missing from event data, it will raise a KeyError at line 117 before any None check can execute.

## Fix Focus Areas
- src/sentry/event_manager.py[696-698]

Consider either:
1. Removing the None check and relying on existing error handling
2. Adding proper try-except around datetime access if missing timestamps need to be handled gracefully

)
job["groups"] = []

Expand Down
2 changes: 1 addition & 1 deletion src/sentry/issues/ingest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def process_occurrence_data(data: dict[str, Any]) -> None:


def hash_fingerprint(fingerprint: list[str]) -> list[str]:
return [md5(part.encode("utf-8")).hexdigest() for part in fingerprint]
return [md5(part.encode("utf-8")).hexdigest() if part else "" for part in fingerprint]


class IssueArgs(TypedDict):
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/notifications/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def get_reason_context(extra_context: Mapping[str, Any]) -> MutableMapping[str,
"""Get user-specific context. Do not call get_context() here."""
reason = extra_context.get("reason", 0)
return {
"reason": GroupSubscriptionReason.descriptions.get(reason, "are subscribed to this issue")
"reason": GroupSubscriptionReason.descriptions.get(reason, "subscribed")
Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025 •
edited by AdamWalkerQodo
Loading

Choose a reason for hiding this comment

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

Action Required

2.Incorrect Default for Notification Reason □ 🐞 Bug

The default value for the notification reason changed from "are subscribed to this issue" to "subscribed". Looking at the GroupSubscriptionReason.descriptions dictionary, all the description values are complete sentences starting with verbs (e.g., "have commented on this issue", "have been assigned to this issue"). The new default "subscribed" is grammatically inconsistent with the existing descriptions and will result in malformed notification text. The descriptions are meant to complete a sentence like "You [reason]".

Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
The default notification reason text was changed to "subscribed", which is grammatically inconsistent with all other reason descriptions. All descriptions in GroupSubscriptionReason.descriptions are complete verb phrases that follow "You" in a sentence.

## Issue Context
The reason text is used in notification templates to explain why a user is receiving the notification. The format is "You [reason]". All existing reasons follow this pattern with complete verb phrases.

## Fix Focus Areas
- src/sentry/notifications/helpers.py[113-113]

}


Expand Down
2 changes: 1 addition & 1 deletion src/sentry/quotas/redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ def refund(
return_key = self.get_refunded_quota_key(
self.__get_redis_key(quota, timestamp, shift, project.organization_id)
)
pipe.incr(return_key, quantity)
pipe.decr(return_key, quantity)
Copy link

@qodo-code-review qodo-code-review bot Nov 27, 2025 •
edited by AdamWalkerQodo
Loading

Choose a reason for hiding this comment

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

Action Required

1. Quota Refund Logic Inverted □ 🐞 Bug

The refund method changed from pipe.incr(return_key, quantity) to pipe.decr(return_key, quantity). This inverts the refund logic. The refund key is meant to track how many events have been refunded, and this value is subtracted from the usage count (line 171: int(result.value or 0) - int(refund_result.value or 0)). By using decr instead of incr, the refund key becomes negative, which when subtracted from usage actually increases the reported usage instead of decreasing it. This breaks the quota refund mechanism entirely.

Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue Description
The quota refund logic has been inverted by changing from `incr` to `decr`. The refund key tracks the count of refunded events, which is subtracted from total usage. Using `decr` makes this value negative, causing refunds to increase usage instead of decrease it.

## Issue Context
When events are refunded (e.g., after being discarded due to rate limiting or errors), the system should decrease the quota usage. The refund key stores a positive count of refunded items, which is subtracted from the raw usage count in the `get_usage` method.

## Fix Focus Areas
- src/sentry/quotas/redis.py[228-228]

pipe.expireat(return_key, int(expiry))

pipe.execute()
Expand Down
4 changes: 2 additions & 2 deletions static/app/views/issueDetails/groupSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,9 @@ export default function GroupSidebar({

const renderSeenByList = () => {
const {seenBy} = group;
const displayUsers = seenBy.filter(user => activeUser.id !== user.id);
const displayUsers = seenBy.filter(user => activeUser?.id !== user.id);

if (!displayUsers.length) {
if (!displayUsers?.length) {
return null;
}

Expand Down