Support non-utf8 data in collapse by doing lossy conversion

[austinabell]

A project I'm benchmarking has non-utf8 data in the dtrace, and it's quite annoying to have to use iconv to sanitize the file before collapsing. I did this for my own needs, and figured I would PR as it might be useful to others as well.

If you are open to this, I should also add this to other parts of the code that do similar things, but I won't do that preemptively unless go ahead is given (not sure if there is a good reason this is not done)

[jonhoo]

This seems like a very reasonable thing to include to me!

[austinabell]

This seems like a very reasonable thing to include to me!

Awesome! I've made the change to the other options. I did remove the handling inside of the is_applicable function because the parameter is already a &str and I'm not confident of the usage and was never able to trigger an issue there. I also didn't want to change the interface in any way. If you'd like I can try to switch it to &[u8] param, but I haven't had an issue with it yet.

There should be no additional allocations, except in the case of non-utf8 data, and although benchmarks on my system were pretty imprecise, none showed regressions. I also updated the test case which specifically fails on invalid utf8, was this intentional before?

[jonhoo]

Looks good! I don't have a good answer for why we explicitly tested against the error for the non-UTF8 case. That just seems wrong!

[jonhoo]

It looks like your change no longer compiles on Rust 1.40.0, though I'm not sure. And looks like the build logs have since disappeared (sorry it too me so long to get back to this). Could you either try running it locally (cargo +1.40.0 test) or just push an empty commit (git commit --allow-empty) to trigger CI again so we can look at what fails?

[austinabell]

It looks like your change no longer compiles on Rust 1.40.0, though I'm not sure. And looks like the build logs have since disappeared (sorry it too me so long to get back to this). Could you either try running it locally (cargo +1.40.0 test) or just push an empty commit (git commit --allow-empty) to trigger CI again so we can look at what fails?

Ah it does fail locally, but it does also on master, I can look into fixing this later, but don't have time in the very near future

[jonhoo]

Ah, looks like one of our dependencies bumped their MSRV. I'll look into it, thanks!