gRPC support for stenoread about stenographer

[gconnell]

I think that's an excellent idea!

That said, I'd probably prefer if this were just integrated directly into the stenographer binary instead of being a side-car, maybe flag-controlled to determine whether it exposes via the current method? Or just another endpoint?

[jshlbrd]

Agreed 100%. I have something I should be able to share soon for your review!

[jshlbrd]

@gconnell feel free to take a look at this branch whenever you have time: https://github.com/jshlbrd/stenographer/tree/feature/grpc. The gRPC server is implemented as a goroutine if the user supplies a specific config dictionary in steno.conf (see DESIGN.md in the branch for details) and it's designed to never crash stenotype et al. I have some ideas on how to iterate on this in the future (e.g. there are some stenoread queries clients should probably never try to execute, like "tcp" without time modifiers), but for v1, this is fully tested.

Here's a summary of the file changes:

• Added rpc/rpc.go
• Added protobuf/steno.proto
• Changed config/config.go to support Rpc configuration
• Changed stenographer.go to check for Rpc configuration and (if found) run gRPC server as a goroutine
• Updated README and DESIGN docs to describe Rpc functionality

Aside from any potential bugs in the code, I'm curious how the design of this update looks (especially the protobuf).

[aeppert]

This looks solid to me. The only outlying question I can immediately think of, having implemented something similar in the past, just not with gRPC as we discussed @jshlbrd - how does it handle a zero-length PCAP? So if stenographer doesn't find anything. If I recall correctly, the issue there is Stenographer will send back just the PCAP header, which is normally fine, but can cause problems otherwise. If there is an issue with this, I believe I have the changes I made and can submit a follow-up or side-saddle patch to take care of this situation. Basically, it returns a 404 instead of a header plus zero length PCAP data.

[jshlbrd]

@aeppert Good question -- it currently doesn't. However, there are multiple cases when gRPC will return a 0-byte stream:
https://github.com/jshlbrd/stenographer/blob/feature/grpc/rpc/rpc.go#L36-L38
https://github.com/jshlbrd/stenographer/blob/feature/grpc/rpc/rpc.go#L56-L60
https://github.com/jshlbrd/stenographer/blob/feature/grpc/rpc/rpc.go#L61-L65

If we'd like to handle the zero-length PCAP (IIRC this is when the resulting PCAP file is 24bytes, right?), then I'd suggest we do it similarly to those ("return nil"). Would probably make the most sense to do it here, right before the PCAP is chunked for streaming: https://github.com/jshlbrd/stenographer/blob/feature/grpc/rpc/rpc.go#L67

An alternate solution to returning nothing is to always return at least one message with a status code that describes what happened. This would alter the protobuf, but would not be difficult to add as long as we could define the status codes (e.g. successful extraction, successful extraction with truncation, general failure, zero-length PCAP, etc.)

For now, I've addressed this in this commit: jshlbrd@49dee77

[jshlbrd]

PR #202 is submitted, please feel free to review, comment, and make suggestions as you have time. Thanks guys!

[aeppert]

@gconnell This looks solid to me. I would say this meets a solid MVP and a nice step forward. Thoughts?