Skip to content
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.

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

ROX-28557: Collector should send complete networking delta when the config is updated #2068

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JoukoVirtanen
Copy link
Contributor

@JoukoVirtanen JoukoVirtanen commented Mar 20, 2025

Description

Currently if external IPs is disabled and then it is switched to being enabled, the unnormalized connection will be reported open at the next scrape interval, but the normalized connection will only be reported closed after the afterglow period is expired. A similar problem occurs if external IPs is enabled and then is switched to being disabled. In the above example we would like the normalized connection to be reported closed at the same time that the unnormalized connection is reported open.

To accomplish this the afterglow period is set to be slightly less than the scrape interval when the state of the setting for external IPs is changed. This is done so that short lived connections that were opened and closed during the previous scrape interval will be reported as being open, and connections that went from being normalized to unnormalized from one scrape to the next will have the normalized version reported as being closed. Connections that were closed during previous scrape intervals, but are not passed their afterglow period will be reported as being closed. It may be a bit arbitrary that short lived connections within the previous scrape interval will not be reported closed, but connections that were closed during previous scrape intervals will be reported closed.

Alternatives that were considered included clearing the old state of the connections when the runtime config is changed. This would however, result in the normalized connection not being reported as closed as it is would not be in the old or new states. This would be okay if sensor knew that the messages sent constituted a state and not a delta.

Another alternative that was considered was turning off afterglow for scrapes in which the state of the external IPs feature had changed. This didn't work, because short lived connections would be reported as closed, which is not desired.

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Automated testing

  • Added unit tests
  • Added integration tests
  • Added regression tests

If any of these don't apply, please comment below.

Testing Performed

TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.

For more details, ref the Confluence page about this section.

Sorry, something went wrong.

@JoukoVirtanen JoukoVirtanen requested a review from a team as a code owner March 20, 2025 23:15
if (enable_afterglow_ && prevEnableExternalIPs != enableExternalIPs) {
afterglow_period_micros = time_micros - time_at_last_scrape - 1;
CLOG(INFO) << "Enable external IPs changed from " << prevEnableExternalIPs << " to " << enableExternalIPs;
CLOG(INFO) << "Setting afterglow_period_micros to " << afterglow_period_micros << " for one scrape";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These logging statements will be removed.

@JoukoVirtanen JoukoVirtanen marked this pull request as draft March 20, 2025 23:16
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 27.27273% with 8 lines in your changes missing coverage. Please review.

Project coverage is 27.55%. Comparing base (53e3a8d) to head (0dc1cc1).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
collector/lib/NetworkStatusNotifier.cpp 27.27% 5 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2068   +/-   ##
=======================================
  Coverage   27.54%   27.55%           
=======================================
  Files          89       89           
  Lines        5725     5731    +6     
  Branches     2547     2550    +3     
=======================================
+ Hits         1577     1579    +2     
- Misses       3481     3483    +2     
- Partials      667      669    +2     
Flag Coverage Δ
collector-unit-tests 27.55% <27.27%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants