Testing the gh actions workflow for contribution violations (no discussion)#5901
Testing the gh actions workflow for contribution violations (no discussion)#5901gh-action-test wants to merge 1 commit intogetsentry:masterfrom
Conversation
sdk-maintainer-bot
bot
added
missing-maintainer-discussion
|
This PR has been automatically closed. The referenced issue does not show a discussion between you and a maintainer. To avoid wasted effort on both sides, please discuss your proposed approach in the issue first and wait for a maintainer to respond before opening a PR. Please review our contributing guidelines for more details. |
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Langchain
Bug Fixes 🐛Ci
Openai
Other
Documentation 📚
Internal Changes 🔧Langchain
Openai
Other
Other
🤖 This preview updates automatically when you update the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| class LogBatcher(Batcher["Log"]): | ||
| MAX_BEFORE_FLUSH = 100 | ||
| MAX_BEFORE_FLUSH = 1_000 |
There was a problem hiding this comment.
Flush threshold equals drop threshold, eliminating burst headroom
Medium Severity
MAX_BEFORE_FLUSH is now equal to MAX_BEFORE_DROP (both 1_000), which eliminates the buffer between flushing and dropping. In Batcher.add(), the flush event is set only when the buffer reaches 1_000 items, but any new items arriving before the flush thread clears the buffer are immediately dropped. Previously, flushing triggered at 100, leaving 900 items of headroom. The _span_batcher.py comment explicitly states "MAX_BEFORE_FLUSH should be lower than MAX_BEFORE_DROP" for exactly this reason, and MetricsBatcher uses a 10x gap between the two thresholds.
|
|
||
| class LogBatcher(Batcher["Log"]): | ||
| MAX_BEFORE_FLUSH = 100 | ||
| MAX_BEFORE_FLUSH = 1_000 |
There was a problem hiding this comment.
Test-only code change accidentally included in production code
High Severity
The PR title explicitly states this is "Testing the gh actions workflow for contribution violations," and the description references a "fake issue." The MAX_BEFORE_FLUSH change from 100 to 1_000 appears to be a throwaway modification made solely to produce a diff for workflow testing, not an intentional production change. Merging this would silently alter log batching behavior in production.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| MAX_BEFORE_FLUSH = 1_000 | ||
| MAX_BEFORE_DROP = 1_000 |
There was a problem hiding this comment.
Bug: Setting MAX_BEFORE_FLUSH equal to MAX_BEFORE_DROP eliminates the safety buffer, causing logs to be dropped immediately when the flush threshold is met under load.
Severity: HIGH
Suggested Fix
Increase MAX_BEFORE_DROP to a value higher than MAX_BEFORE_FLUSH to restore the safety margin. For example, set MAX_BEFORE_DROP to 2_000 or 10_000, following the pattern of other batchers in the codebase.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry_sdk/_log_batcher.py#L13-L14
Potential issue: The change sets `MAX_BEFORE_FLUSH` to `1_000` but leaves
`MAX_BEFORE_DROP` at the same value. In the `Batcher.add()` method, when the buffer size
reaches this threshold, a flush is triggered. However, any new log items that arrive
before the flush thread can drain the buffer will be immediately dropped because the
buffer size is still `>= MAX_BEFORE_DROP`. This eliminates the safety margin intended to
handle items arriving during a flush operation, a principle explicitly documented in
other parts of the codebase like `SpanBatcher`. Under any meaningful log burst, this
will cause logs to be silently dropped.
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
Description
Issues
fake issue
real issue without discussion
Reminders
tox -e linters.feat:,fix:,ref:,meta:)