Normalize connections before sending them to sensor #277
Conversation
|
|
||
| state_.swap(new_state); |
There was a problem hiding this comment.
Hmm, this is weird. new_state and state_ will no longer be in sync, right?
There was a problem hiding this comment.
Ah, I guess new_state is a misnomor, it's now fetched_state. Will change. Or where there any semantic concerns?
There was a problem hiding this comment.
Yeah, that's fine. Btw, will we ever call without normalize? If not, we can just keep the state normalized all the time.
There was a problem hiding this comment.
Nope, we cannot keep the state normalized, as that would not allow us to determine when a (normalized) connection is still open, since we lose track of the raw connections. This could possibly be worked around, but requires substantial changes.
Concretely, if we see:
1. Open 1.2.3.4:54321 -> 5.6.7.8:443 [normalized: 0.0.0.0:0 -> 5.6.7.8:443]
2. Open 1.2.3.4:65432 -> 5.6.7.8:443 [normalized: 0.0.0.0:0 -> 5.6.7.8:443]
3. Close 1.2.3.4:65432 -> 5.6.7.8:443 [normalized: 0.0.0.0:0 -> 5.6.7.8:443]
And we keep the state as normalized, how do you know that 0.0.0.0:0 -> 5.6.7.8:443 is still active? Even if you do some form of counting, it is not robust against cases where you see a close for a connection that you never saw being opened (e.g., due to drops).
misberner
force-pushed
the
mi-normalize-connections
branch
from
0cea9c7 to
8d725c0
Compare
Results for Performance Benchmarks on build #39231
|
| @@ -35,22 +35,29 @@ namespace collector { | |||
| // ConnStatus encapsulates the status of a connection, comprised of the timestamp when the connection was last seen | |||
| // alive (in microseconds since epoch), and a flag indicating whether the connection is currently active. | |||
| class ConnStatus { | |||
| private: | |||
There was a problem hiding this comment.
Can we move the private block below up here too?
There was a problem hiding this comment.
The reason this is up here is that we are declaring a static constant. The privat block below declares a constructor and an instance field. I would prefer to keep it in that order - it's not quite compliant with the google style guide which says to always put public first, but I think the code is easier to read if we put the definition of the constant first.
collector/lib/ConnTracker.h
Outdated
| public: | ||
| ConnStatus() : data_(0UL) {} | ||
| ConnStatus(int64_t microtimestamp, bool active) : data_((microtimestamp & ~0x1UL) | ((active) ? 0x1 : 0x0)) {} | ||
| ConnStatus(int64_t microtimestamp, bool active) : data_((static_cast<uint64_t>(microtimestamp) >> 1) | ((active) ? kActiveFlag : 0UL)) {} |
There was a problem hiding this comment.
Doesn't doing this clobber the last bit of microtimestamp?
There was a problem hiding this comment.
Yup - that was the case even before. 1 microsecond isn't really meaningful.
There was a problem hiding this comment.
Actually, have decided to just drop the highest bit - 2^63 microseconds are roughly 300k years.
collector/lib/ConnTracker.h
Outdated
|
|
||
| void SetActive(bool active) { | ||
| if (active) data_ |= 0x1UL; | ||
| else data_ &= ~0x1UL; | ||
| if (active) data_ |= kActiveFlag; |
There was a problem hiding this comment.
Consider factoring out these two lines from SetActive and WithStatus
|
|
||
| state_.swap(new_state); |
There was a problem hiding this comment.
Yeah, that's fine. Btw, will we ever call without normalize? If not, we can just keep the state normalized all the time.
collector/lib/ConnTracker.h
Outdated
| if (active) new_data |= kActiveFlag; | ||
| else new_data &= ~kActiveFlag; | ||
| return ConnStatus(new_data); | ||
| return ConnStatus(MakeActive(new_data, active)); |
There was a problem hiding this comment.
Nit: can just pass data_ here directly.
misberner
force-pushed
the
mi-normalize-connections
branch
from
5e5fbe3 to
01bb888
Compare
misberner
force-pushed
the
mi-normalize-connections
branch
from
01bb888 to
f249fc4
Compare
Shrink the state we are sending by normalizing connections as follows: