search

quiclog / pcap2qlog

A tool to convert .pcap and .pcapng files into qlog files
MIT License
26 stars 7 forks source link

allow formatting the time in microseconds in the qlog output (stays in milliseconds by default) #5

Closed francoismichel closed 3 years ago

francoismichel commented 3 years ago

Hello,

This pull requests adds the possibility to have a microsecond granularity for the timestamps present in the final qlog file. We can now obtain a microsecond granularity from the CLI by using the --timeunit=us CLI parameter.

By default, it behaves exactly as before, i.e. the default granularity is still in milliseconds (same as using --timeunit=ms).

If you are OK with the functionnality brought by this PR, do not hesitate to let me know if you want me to update the code for whatever reason, including coding style. :-)

François

rmarx commented 3 years ago

Hello @francoismichel,

Thanks for another PR, keep 'em coming :)

For this one, I'm not entirely sure why you want this explicitly. In fact, the option to use microsecond resolution has currently been removed from qlog draft-02 for simplicity. In that setup, you can still log in microsecond resolution of course, it's just more digits after the comma... If you have a solid reason for requiring us, maybe that change needs to be reverted as well.

Secondly, in terms of code structure, I feel it might be time to move towards a more "parameter object" approach (https://refactoring.guru/introduce-parameter-object). Not for all parameters probably, but for some of the config things (your previous logUnknownFramesFields for example would fit in there as well). I don't think it's 100% necessary yet, but if you ever add another boolean like this, I'd take that approach.

So, I'm going to keep this open for now to get more insight into the need for us.

francoismichel commented 3 years ago

Hello,

In that setup, you can still log in microsecond resolution of course, it's just more digits after the comma...

Okay, I didn't know that the timestamp in qlog could be a float, sorry for that. :-)

Currently, with the ms resolution, pcap2qlogs gives integer timestamps and so we loose quite a bit of information when several packets arrive during the same millisecond. I guess that this PR can be closed and we can ensure not loosing the digits after the comma (maybe by not rounding the number in line 194 of ParserPCAP.ts ?).

rmarx commented 3 years ago

Yeah, it's a valid point you make. Before (pre draft-02), the timestamps were indeed somewhat expected to be integers (though it didn't matter in qvis), but they've been changed to doubles in draft-02 to make this type of thing easier (https://quiclog.github.io/internet-drafts/draft02/draft-marx-qlog-main-schema.html#section-3.4). I was planning to do a pass to update pcap2qlog and other tools to draft-02 when that's done (we're fixing the last issues at this stage), but would accept a new PR removing the Math.round sooner :)