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

Add streaming option #109

Closed
rmarx opened this issue Jul 29, 2020 · 8 comments
Closed

Add streaming option #109

rmarx opened this issue Jul 29, 2020 · 8 comments

Comments

@rmarx
Copy link
Contributor

rmarx commented Jul 29, 2020

Current thinking is to allow http://ndjson.org and to log the qlog header information as a separate object on top.
This would remove the possibility to log multiple traces in the same qlog file if it uses the streaming option, so users would need to use group_id in those cases.

Would also require the addition of a "format" field to the qlog metadata header (though we probably need that anyway if we want to keep support for other JSON-based optimizations like event_fields.

We first need to check how this plays with oboe.js in qvis though, to see if we need to add another parser in the mix.

See also #106 and #2 for more discussion.

For comparison:

Draft-01 would look like this (if, like me, JSON serializers would be too lazy to add quotes):
{
    version: draft-01,
    traces: [
        {
             vantage_point: client,
             common_fields: [ ... ],
             event_fields: [ ... ], 
             events: [
                 [ 55, transport, packet_sent, { ... } ],
                 [ 66, transport, packet_received, { ... } ],
                 ...
             ]
        }
    ]
}

Draft-02 would be more like:
{
    version: draft-02,
    format: "ndjson",
    trace: {
             vantage_point: client,
             common_fields: [ ... ]
     }
}
{ time: 55, name: "transport:packet_sent", data: { ... } }
{ time: 66, name: "transport:packet_received", data: { ... } }
...

CC @marten-seemann @huitema

@marten-seemann
Copy link
Collaborator

I like this idea. In the interest of keeping parsers simple, I would be supportive of completely replacing the current qlog format with this.
As I've said elsewhere, I don't think that the complexity that comes with putting multiple traces into the same file justifies the complexity.

@nibanks
Copy link
Member

nibanks commented Jul 29, 2020

I agree with @marten-seemann. We should optimize for the primary case, which is definitely single machine/trace.

@huitema
Copy link

huitema commented Jul 29, 2020

Streaming would definitely be useful for me too. Currently, I have to use a two stage approach to solve that: stream-supporting binary log, then convert to qlog once I am sure the end is reached. And yes, all usage is single machine/trace.

@LPardue
Copy link
Member

LPardue commented Jul 29, 2020

support this, would be good to see a short example of the difference between new and old formats so I can start planning

@rmarx
Copy link
Contributor Author

rmarx commented Jul 30, 2020

I've updated the issue with an example, PTAL.

I'm highly skeptical that that type of thing would be de-serializable automatically with something like @LPardue's serde, but a girl can dream.

@LPardue
Copy link
Member

LPardue commented Jul 30, 2020

Thanks. FWIW, the current qlog format is effectively not deserializable using serde either, so nothing would be lost.

edit: hmm, actually the new format might make it more straightforward to use a semi-automated deserializer

@rmarx
Copy link
Contributor Author

rmarx commented Aug 27, 2020

Initial support for "newline delimited JSON" qlog streaming is now in qvis per quiclog/qvis@8504c0e

An example draft-02 NDJSON file can be found in attachment (should load just fine in the live qvis atm).
Now that I'm certain this can be done without major performance issues, I will move forward with this for qlog draft-02.

In the meantime, further feedback based on the example file is of course welcome.

newline_delimited.zip

rmarx pushed a commit that referenced this issue Aug 31, 2020
- Removes the event_fields optimization. Relates to #30. Fixes #89.

- Removes several points of complexity wrt the group_id field, as they were not being used in practice.

- Makes JSON the default serialization option. Fixes #101.

- Adds a proper streaming option with NDJSON. Relates to #106. Fixes #109, #2.

- Generally tightens the text and adds more examples.
@rmarx
Copy link
Contributor Author

rmarx commented Sep 5, 2020

This issues should have been resolved by the choice of NDJSON as streaming format as of f5db7cd

@rmarx rmarx closed this as completed Sep 5, 2020
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

No branches or pull requests

5 participants