revert browser events query to the previous state

[dinmukhamedm]

Important

Reverts browser events query logic to previous state, removing chunking and adjusting backoff strategy in mod.rs.

• Behavior:
  • Reverts browser events query logic in inner_process_browser_events to previous state, removing event chunking.
  • Changes backoff strategy: initial delay from 0.5s to 1s, with retries up to 60s.
• Code Changes:
  • Removes max_batch_size and chunking logic for event batches.
  • Simplifies query construction by directly appending values for each event.
  • Adjusts error handling to log and retry on insertion failure.

^{This description was created by for eecbe32. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 02a6b8e in 2 minutes and 0 seconds

More details

• Looked at 190 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. app-server/src/browser_events/mod.rs:68

• Draft comment:
  Removed batching: The previous version used an env var to split large event batches, but now the query builds placeholders for all events at once. This could lead to excessively long queries if batch size grows large. Consider reintroducing batching to avoid potential performance or server limits issues.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The comment raises a valid concern about query size scaling with batch size. However, there are mitigating factors: 1) ClickHouse has async_insert enabled 2) The code is using parameter binding which is generally safe 3) There's no clear evidence of what a problematic batch size would be 4) The original batching size of 64 seems arbitrary 5) If there are server limits, they would likely cause errors that are already handled by the retry logic.
  I might be underestimating the impact of very large batches on memory usage or network overhead. The original batching may have been added based on real production issues.
  While large batches could be problematic, the comment is speculative without concrete evidence of issues. The code has error handling and retries, so real problems would surface through monitoring rather than needing preemptive fixes.
  Delete the comment because it is speculative about potential issues without clear evidence that batching is necessary. If batch size becomes a real problem, it will be apparent through monitoring and can be addressed then.

2. app-server/src/browser_events/mod.rs:95

• Draft comment:
  The TODO regarding cloning event data remains. It suggests potential inefficiency with temporary lifetimes. Consider revisiting whether the clone is necessary or if the lifetime issues can be resolved without cloning.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

3. app-server/src/browser_events/mod.rs:126

• Draft comment:
  The backoff configuration now starts with a 1000ms delay compared to the previous 500ms. Confirm that this change is intended as part of the reversion.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment violates one of our key rules - it's asking the author to "confirm" their intention. These types of verification questions are explicitly listed as not useful. The change in backoff timing is a minor implementation detail, and if it was unintended, it would be caught during testing. The author likely considered this when making the change.
  The backoff timing could potentially impact system performance and reliability, so maybe it deserves extra scrutiny?
  While backoff timing is important, asking for confirmation doesn't add value. If there were specific concerns about the timing, those should be raised directly rather than asking the author to verify their intention.
  The comment should be deleted as it violates our rule against asking authors to confirm their intentions. If there are specific concerns about the backoff timing, they should be raised directly.

4. app-server/src/browser_events/mod.rs:60

• Draft comment:
  Removal of batch splitting (using BROWSER_EVENTS_MAX_BATCH_SIZE) may cause issues if batch.events is very large. Confirm that oversized batches are acceptable or reintroduce chunking.
• Reason this comment was not posted:
  Marked as duplicate.

5. app-server/src/browser_events/mod.rs:126

• Draft comment:
  The backoff initial interval is now 1000ms instead of 500ms. Verify that this change is intentional.
• Reason this comment was not posted:
  Marked as duplicate.

6. app-server/src/browser_events/mod.rs:68

• Draft comment:
  Building the SQL query by concatenating placeholders for each event could become inefficient (or hit parameter limits) for very large batches. Ensure the expected batch size is controlled.
• Reason this comment was not posted:
  Marked as duplicate.

7. app-server/src/browser_events/mod.rs:95

• Draft comment:
  Revisit the TODO regarding cloning event data. Consider if the clone can be avoided to reduce potential performance overhead.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The existing TODO is already clear and explains both the issue (cloning data) and why it's currently necessary (lifetime issues). The suggested rewording doesn't add any new information or clarity. The comment is about changed code (the batch insert logic was modified), but the suggestion is purely stylistic and doesn't improve understanding.
  The cloning issue could be a real performance concern, and having a clearer TODO might help future developers address it.
  The existing TODO already clearly communicates both the issue and the reason for the current implementation. The suggested rewording is just style preference.
  Delete the comment as it only suggests rewording an already clear and informative TODO without adding any technical value.

8. app-server/src/browser_events/mod.rs:140

• Draft comment:
  Insertion failures are logged but the message is always acked. Confirm if acking should occur when insertion fails after all retries.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

Workflow ID: wflow_XiqcsIaafIqaRjhC

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on eecbe32 in 44 seconds

More details

• Looked at 32 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. app-server/src/browser_events/mod.rs:134

• Draft comment:
  Good: The success branch now explicitly acks the message, ensuring proper completion when retries succeed.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. app-server/src/browser_events/mod.rs:140

• Draft comment:
  Consider verifying that using acker.reject(false) is the intended behavior for rejecting a message (i.e. whether it requeues or discards it).
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to verify their intention regarding the use of acker.reject(false). This violates the rule against asking the author to confirm their intention or ensure behavior is intended. The comment does not provide a specific suggestion or point out a clear issue.

3. app-server/src/browser_events/mod.rs:134

• Draft comment:
  Using a match expression on the result of the retry improves clarity compared to mapping errors. This change clearly separates the success and failure pathways.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

4. app-server/src/browser_events/mod.rs:140

• Draft comment:
  When retries are exhausted, the message is rejected via acker.reject(false). Confirm that using 'false' (likely indicating no requeue) is the intended behavior, to avoid potential loss of events.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. app-server/src/browser_events/mod.rs:136

• Draft comment:
  In the success branch, the acker.ack() call failure is only logged. Consider whether additional handling (e.g., a retry mechanism or alerting) might be beneficial if acknowledging the message fails.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_oJlySq2mZ3anHV60

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.