ROX-31236: Remove ROX_CAPTURE_INTERMEDIATE_EVENTS flag#17240
Merged
ROX-31236: Remove ROX_CAPTURE_INTERMEDIATE_EVENTS flag#17240
ROX_CAPTURE_INTERMEDIATE_EVENTS flag#17240Conversation
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Remove any remaining dead code paths and unused helper methods, such as the now-unused resetContext and resetLastSentState, to keep the codebase clean.
- Ensure the simplified non-blocking sendToCentral logic handles backpressure correctly and logs dropped events as intended under high-throughput conditions.
- Verify that the refactored Notify methods across components preserve the correct offline-to-online transition behavior without the old feature flag checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove any remaining dead code paths and unused helper methods, such as the now-unused resetContext and resetLastSentState, to keep the codebase clean.
- Ensure the simplified non-blocking sendToCentral logic handles backpressure correctly and logs dropped events as intended under high-throughput conditions.
- Verify that the refactored Notify methods across components preserve the correct offline-to-online transition behavior without the old feature flag checks.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a347526 to
deb0695
Compare
Contributor
|
Images are ready for the commit at 6fcac11. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17240 +/- ##
==========================================
- Coverage 48.86% 48.84% -0.03%
==========================================
Files 2720 2720
Lines 203364 203276 -88
==========================================
- Hits 99373 99284 -89
- Misses 96167 96174 +7
+ Partials 7824 7818 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fa41283 to
4db7f93
Compare
10 tasks
ROX_CAPTURE_INTERMEDIATE_EVENTS flag
4db7f93 to
9ebf8d6
Compare
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The refactored sendToCentral now uses a non-blocking select in all cases, whereas previously the unbuffered path would block until consumption; please verify this change does not lead to unintended message drops under normal and high-load scenarios.
- The simplified Pipeline.Notify no longer handles context recreation or cancellation for offline/online transitions; ensure that this refactor doesn’t introduce context leaks or alter the intended signal timing in edge cases.
- After removing the SensorCapturesIntermediateEvents guard in Queue.Start, the isRunning signal is no longer emitted on Start, so please ensure the queue’s initial state still allows processing without deadlock.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The refactored sendToCentral now uses a non-blocking select in all cases, whereas previously the unbuffered path would block until consumption; please verify this change does not lead to unintended message drops under normal and high-load scenarios.
- The simplified Pipeline.Notify no longer handles context recreation or cancellation for offline/online transitions; ensure that this refactor doesn’t introduce context leaks or alter the intended signal timing in edge cases.
- After removing the SensorCapturesIntermediateEvents guard in Queue.Start, the isRunning signal is no longer emitted on Start, so please ensure the queue’s initial state still allows processing without deadlock.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This removes the ROX_CAPTURE_INTERMEDIATE_EVENTS feature flag that has been enabled by default since Release 4.3 (February 2024). The flag enabled offline v3 behavior where Sensor buffers events during Central disconnects. This is now the standard operational mode. Changes: - Removed feature flag definition from pkg/features/list.go - Network flow manager: Always use buffered channels and continue enrichment during offline mode - Process signal pipeline: Always use long-lived contexts that persist across disconnects - Detector: Always use buffered queues and handle pause/resume on connection events - Removed EnrichmentReasonEpFeatureDisabled (no longer returned) - Updated integration tests to remove flag setup - Removed unused features package imports from 9 files All event buffering and offline operation functionality remains fully operational. This PR is stacked on #17236 (ROX_PREVENT_SENSOR_RESTART_ON_DISCONNECT removal). Partially generated with AI assistance.
- Updated comments to reflect feature is always enabled - Removed obsolete test cases for disabled feature - Skipped tests specific to v1/v2 behavior (feature disabled) - Removed EnrichmentReasonEpFeatureDisabled constant
8e26560 to
5c6bd5b
Compare
Contributor
Author
We are aware that the channel will start dropping messages once it is full. It's been running like this for months.
We use a long-lived detection context to not stop detection in offline mode. The context for sending to Central while offline will still be canceled so that messages are not sent.
Fixed in 2431bb2 by starting the queue in running mode. aand reverted in 6fcac11 due to failing tests. Apparently the queue is expected to start in paused state. Let's keep it like this. |
This reverts commit 2431bb2.
|
@vikin91: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
janisz
approved these changes
Oct 16, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human here
This has been created fully by AI.
t.Skip. This was followed by running an unused-code check.Description
This PR removes the
ROX_CAPTURE_INTERMEDIATE_EVENTSfeature flag which has been enabled by default since Release 4.3 (February 2024) - approximately 1.7 years in production. Since this removal depends on the connection retry mechanism from #17236, both removals are included in this PR.📋 Detailed Implementation Plan: REMOVAL_PLAN_CAPTURE_INTERMEDIATE_EVENTS.md
Why it's safe to remove:
Removal Sequence Justification
This PR combines the first and third PRs of 4 in the feature flag removal sequence.
Position in sequence: FIRST + THIRD (Foundation + Layer 2)
Why this order:
ROX_PREVENT_SENSOR_RESTART_ON_DISCONNECTflag #17236)ROX_PREVENT_SENSOR_RESTART_ON_DISCONNECTflag #17236 (connection retry) provides the foundation for reliable operationDependencies:
ROX_PREVENT_SENSOR_RESTART_ON_DISCONNECTflag #17236 (ROX_PREVENT_SENSOR_RESTART_ON_DISCONNECT) - foundation layerMerge Strategy: #17236 should be merged first for clean git history. After #17236 merges, this PR will only show the ROX_CAPTURE_INTERMEDIATE_EVENTS changes.
Reference: See attached
REMOVAL_PLAN_CAPTURE_INTERMEDIATE_EVENTS.mdfor complete analysis.Files modified:
ROX_PREVENT_SENSOR_RESTART_ON_DISCONNECTflag #17236)Suggested reviewers: (AI got that wrong)
User-facing documentation
Testing and quality
Automated testing
How I validated my change