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

cli: collect: sort: Enable gzip compression #489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkp-rh
Copy link

@mkp-rh mkp-rh commented Jan 31, 2025

Add the --gzip parameter to allow output to be gzip compressed. Also add support for reading both gzip and uncompressed files transparently.

This feature was suggested by @igsilya

Add the --gzip parameter to allow output to be gzip compressed. Also add
support for reading both gzip and uncompressed files transparently.

Signed-off-by: Mike Pattrick <[email protected]>
@amorenoz amorenoz added the run-functional-tests Request functional tests to be run by CI label Feb 3, 2025
@amorenoz amorenoz self-requested a review February 3, 2025 20:49
@amorenoz amorenoz self-assigned this Feb 3, 2025
@amorenoz
Copy link
Contributor

amorenoz commented Feb 4, 2025

This is not a full review. I wanted to relate this feature with the discussions that took place in #454.

In essence, should be code stuff into retis or maybe have retis place nice with UNIX pipes. In this particular case, it would avoid us the need to re-implement "gzip" inside retis.

For example:

With this PR:

# ./target/debug/retis -p ifdump collect --allow-system-changes -o --gzip  --cmd  "iperf3 -c 172.200.0.3"
Applying profile ifdump
Collector(s) started: ovs, skb, skb-drop, skb-tracking, nft, ct
12 probe(s) loaded
Command returned (exit status: 0), terminating ...
44938 event(s) processed
WARN  lost 2454649 event(s) from net:netif_receive_skb
WARN  lost 8077 event(s) from skb:kfree_skb
WARN  lost 719924 event(s) from ovs_dp_upcall
WARN  lost 3290281 event(s) from ovs_execute_actions
WARN  lost 2419768 event(s) from net:net_dev_start_xmit
WARN  lost 747254 event(s) from openvswitch:ovs_dp_upcall
WARN  lost 1666262 event(s) from openvswitch:ovs_do_execute_action
WARN  lost 3406 event(s) from __nft_trace_packet
WARN  total events lost: 11309621
# ls -lh retis.data 
-rw-r--r--. 1 root root 4.1M Feb  4 10:13 retis.data
# time ./target/debug/retis print | wc -l 
150827

real    0m6.353s
user    0m6.263s
sys     0m0.204s

With pipes and a slightly hacked main branch[1]:

# ./target/debug/retis -p ifdump collect --allow-system-changes -o /dev/stdout --cmd  "iperf3 -c 172.200.0.3" | gzip -c > retis_compressed.gz                                            

Applying profile ifdump
Collector(s) started: ct, skb-drop, ovs, nft, skb, skb-tracking
12 probe(s) loaded
Command returned (exit status: 0), terminating ...
116450 event(s) processed
WARN  lost 3144947 event(s) from ovs_execute_actions
WARN  lost 715180 event(s) from openvswitch:ovs_dp_upcall
WARN  lost 1594509 event(s) from openvswitch:ovs_do_execute_action
WARN  lost 7360 event(s) from skb:kfree_skb
WARN  lost 4507 event(s) from __nft_trace_packet
WARN  lost 2348325 event(s) from net:netif_receive_skb
WARN  lost 689302 event(s) from ovs_dp_upcall
WARN  lost 2312317 event(s) from net:net_dev_start_xmit
WARN  total events lost: 10816447
# ls -lha retis_compressed.gz
-rw-r--r--. 1 root root 12M Feb  4 10:08 retis_compressed.gz
# time gzip -cd retis_compressed.gz | ./target/debug/retis print  --file-type events /dev/stdin | wc -l                                                                                  
390643

real    0m15.161s
user    0m15.237s
sys     0m0.587s

By using pipes, retis is able to capture more than 2.5x events

The compression rate is similar although flate2 wins. Not sure why is that but maybe we could look at size of BufWriter?

Still, the pipe option has a decent

# gzip -cd retis.data > retis_uncompressed_with_flate.json
# ls -lh retis.data retis_uncompressed_with_flate.json
-rw-r--r--. 1 root root 4.1M Feb  4 10:13 retis.data
-rw-r--r--. 1 root root  36M Feb  4 10:38 retis_uncompressed_with_flate.json

# gzip -cd retis_compressed.gz > retis_uncompressed_with_pipes.json 
# ls -lh retis_compressed.gz retis_uncompressed_with_pipes.json
-rw-r--r--. 1 root root 12M Feb  4 10:08 retis_compressed.gz
-rw-r--r--. 1 root root 94M Feb  4 10:38 retis_uncompressed_with_pipes.json

Still, having a slightly less compression rate but more than doubling the number of events is a fair trade of IMHO.

Of course this assumes you have a server where gzip can maybe use a different CPU...

[1] I quickly hacked a --file-type option that avoids guessing the filetype which requires us to seek the input file which is obviously not possible with pipes

@mkp-rh
Copy link
Author

mkp-rh commented Feb 4, 2025

By using pipes, retis is able to capture more than 2.5x events

This makes sense. With my my patch the compression happens in the main thread, not ideal! Having writes happen in a different thread is a better solution even without builtin gzip, as writes could block for an indeterminate amount of time in any case.

Re #454 another possible option would be to add support for serde_cbor. The interface is very similar, so not a lot of code would have to change. And one benchmark showed a 3x improvement on serialization and 30% improvement on deserialization.

But probably the best long term solution is to have a separate thread do read/write and encode/decode. This patch was my first code written in Rust, so I don't know what that would look like, but I can try to tackle it this weekend.

@amorenoz
Copy link
Contributor

amorenoz commented Feb 5, 2025

By using pipes, retis is able to capture more than 2.5x events

This makes sense. With my my patch the compression happens in the main thread, not ideal! Having writes happen in a different thread is a better solution even without builtin gzip, as writes could block for an indeterminate amount of time in any case.

Re #454 another possible option would be to add support for serde_cbor. The interface is very similar, so not a lot of code would have to change. And one benchmark showed a 3x improvement on serialization and 30% improvement on deserialization.

Binary representation is something I've wanted to explore for some time. I wanted to wait until we had python bindings because in my mind, parsing json in python was always a "plan B". Now, even the python library published in "pip" uses rust to parse so this change could very well be completely transparent to users.

@atenart thoughts?

But probably the best long term solution is to have a separate thread do read/write and encode/decode. This patch was my first code written in Rust, so I don't know what that would look like, but I can try to tackle it this weekend.

We've discussed this also a few times. An argument keeps popping up: let's keep it simple; keep the collection as simple as possible and do the rest as post-processing. I actually must have patches for this, somewhere...

@atenart
Copy link
Contributor

atenart commented Feb 6, 2025

By using pipes, retis is able to capture more than 2.5x events

This makes sense. With my my patch the compression happens in the main thread, not ideal! Having writes happen in a different thread is a better solution even without builtin gzip, as writes could block for an indeterminate amount of time in any case.
Re #454 another possible option would be to add support for serde_cbor. The interface is very similar, so not a lot of code would have to change. And one benchmark showed a 3x improvement on serialization and 30% improvement on deserialization.

Binary representation is something I've wanted to explore for some time. I wanted to wait until we had python bindings because in my mind, parsing json in python was always a "plan B". Now, even the python library published in "pip" uses rust to parse so this change could very well be completely transparent to users.

@atenart thoughts?

Yes, that makes sense. I have the similar feeling: JSON was important to keep as long as it was the only option for post-processing. But now we have Python support and this is the preferred way. We also never promised to keep the event file format stable so it's fine to improve it.

Although if that does not add too many quirks we could support JSON and other formats. We can then just make the binary one the default.

But probably the best long term solution is to have a separate thread do read/write and encode/decode. This patch was my first code written in Rust, so I don't know what that would look like, but I can try to tackle it this weekend.

We've discussed this also a few times. An argument keeps popping up: let's keep it simple; keep the collection as simple as possible and do the rest as post-processing. I actually must have patches for this, somewhere...

IIRC this might not be possible w/o so prerequisites, but I might not remember well. One that front I have multiple ideas about improving performances (startup + run time) and I'll likely have a look at those after the current set of PRs I have open.

@vlrpl
Copy link
Contributor

vlrpl commented Feb 6, 2025

By using pipes, retis is able to capture more than 2.5x events

This makes sense. With my my patch the compression happens in the main thread, not ideal! Having writes happen in a different thread is a better solution even without builtin gzip, as writes could block for an indeterminate amount of time in any case.
Re #454 another possible option would be to add support for serde_cbor. The interface is very similar, so not a lot of code would have to change. And one benchmark showed a 3x improvement on serialization and 30% improvement on deserialization.

Binary representation is something I've wanted to explore for some time. I wanted to wait until we had python bindings because in my mind, parsing json in python was always a "plan B". Now, even the python library published in "pip" uses rust to parse so this change could very well be completely transparent to users.
@atenart thoughts?

Yes, that makes sense. I have the similar feeling: JSON was important to keep as long as it was the only option for post-processing. But now we have Python support and this is the preferred way. We also never promised to keep the event file format stable so it's fine to improve it.

Although if that does not add too many quirks we could support JSON and other formats. We can then just make the binary one the default.

I agree with this approach. JSON may still prove to be useful.

But probably the best long term solution is to have a separate thread do read/write and encode/decode. This patch was my first code written in Rust, so I don't know what that would look like, but I can try to tackle it this weekend.

We've discussed this also a few times. An argument keeps popping up: let's keep it simple; keep the collection as simple as possible and do the rest as post-processing. I actually must have patches for this, somewhere...

IIRC this might not be possible w/o so prerequisites, but I might not remember well. One that front I have multiple ideas about improving performances (startup + run time) and I'll likely have a look at those after the current set of PRs I have open.

In general bpf/user throughput has room for improvement (w/ file write being the end of the chain for collection), some of them may influence each other (per CPU buffer/event structure/consumers), and among the multiple things discussed, there is also an alignment front to take care of.

@vlrpl
Copy link
Contributor

vlrpl commented Feb 6, 2025

By using pipes, retis is able to capture more than 2.5x events

This makes sense. With my my patch the compression happens in the main thread, not ideal! Having writes happen in a different thread is a better solution even without builtin gzip, as writes could block for an indeterminate amount of time in any case.
Re #454 another possible option would be to add support for serde_cbor. The interface is very similar, so not a lot of code would have to change. And one benchmark showed a 3x improvement on serialization and 30% improvement on deserialization.

Binary representation is something I've wanted to explore for some time. I wanted to wait until we had python bindings because in my mind, parsing json in python was always a "plan B". Now, even the python library published in "pip" uses rust to parse so this change could very well be completely transparent to users.
@atenart thoughts?

Yes, that makes sense. I have the similar feeling: JSON was important to keep as long as it was the only option for post-processing. But now we have Python support and this is the preferred way. We also never promised to keep the event file format stable so it's fine to improve it.
Although if that does not add too many quirks we could support JSON and other formats. We can then just make the binary one the default.

I agree with this approach. JSON may still prove to be useful.

one more thing to keep in mind (that might even turn out to not be a problem at all) is backward compatibility (whether we want to have it to some extent or not) and the binary format should not corner ourselves (ideally, at least no more than the other supported formats :)

But probably the best long term solution is to have a separate thread do read/write and encode/decode. This patch was my first code written in Rust, so I don't know what that would look like, but I can try to tackle it this weekend.

We've discussed this also a few times. An argument keeps popping up: let's keep it simple; keep the collection as simple as possible and do the rest as post-processing. I actually must have patches for this, somewhere...

IIRC this might not be possible w/o so prerequisites, but I might not remember well. One that front I have multiple ideas about improving performances (startup + run time) and I'll likely have a look at those after the current set of PRs I have open.

In general bpf/user throughput has room for improvement (w/ file write being the end of the chain for collection), some of them may influence each other (per CPU buffer/event structure/consumers), and among the multiple things discussed, there is also an alignment front to take care of.

@igsilya
Copy link
Contributor

igsilya commented Feb 6, 2025

Yes, that makes sense. I have the similar feeling: JSON was important to keep as long as it was the only option for post-processing. But now we have Python support and this is the preferred way. We also never promised to keep the event file format stable so it's fine to improve it.
Although if that does not add too many quirks we could support JSON and other formats. We can then just make the binary one the default.

I agree with this approach. JSON may still prove to be useful.

One note here: I find it extremely useful to be able to grep the events.json for things I'm looking for, just to check if they are present or not. For example:

grep -oE 'OVS_DROP_METER|OVS_DROP_CONNTRACK' events.json | sort | uniq -c
     8 OVS_DROP_METER
     1 OVS_DROP_CONNTRACK

This is especially important in situations where retis sort/print takes multiple minutes or even dozens of minutes to parse the file, while grep can find required information in seconds. The gzip archive can be uncompressed if necessary, but having an arbitrary binary format is not really desired, from my point of view.

@atenart
Copy link
Contributor

atenart commented Feb 6, 2025

Yes, that makes sense. I have the similar feeling: JSON was important to keep as long as it was the only option for post-processing. But now we have Python support and this is the preferred way. We also never promised to keep the event file format stable so it's fine to improve it.
Although if that does not add too many quirks we could support JSON and other formats. We can then just make the binary one the default.

I agree with this approach. JSON may still prove to be useful.

One note here: I find it extremely useful to be able to grep the events.json for things I'm looking for, just to check if they are present or not. For example:

grep -oE 'OVS_DROP_METER|OVS_DROP_CONNTRACK' events.json | sort | uniq -c
     8 OVS_DROP_METER
     1 OVS_DROP_CONNTRACK

This is especially important in situations where retis sort/print takes multiple minutes or even dozens of minutes to parse the file, while grep can find required information in seconds. The gzip archive can be uncompressed if necessary, but having an arbitrary binary format is not really desired, from my point of view.

Different use cases having different needs. That makes perfect sense, in addition to backward compatibility to read older event files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-functional-tests Request functional tests to be run by CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants