cmd/ockafka: Use gNMI client instead of openconfig. #22
Conversation
| ) | ||
|
|
||
| var keysFlag = flag.String("kafkakeys", "", | ||
| "Keys for kafka messages (comma-separated, default: the value of -addrs") | ||
| var keyFlag = flag.String("kafkakey", "", |
There was a problem hiding this comment.
This removes the ability to have multiple keys
| @@ -36,33 +36,55 @@ func newProducer(addresses []string, topic, key, dataset string) (producer.Produ | |||
| } | |||
|
|
|||
| func main() { | |||
| username, password, subscriptions, grpcAddrs, opts := client.ParseFlags() | |||
| cfg := &gnmi.Config{} | |||
| flag.StringVar(&cfg.Addr, "addr", "", "Address of gNMI gRPC server with optional VRF name") | |||
There was a problem hiding this comment.
This removes the ability to connect to multiple servers
| ) | ||
|
|
||
| // MessageEncoder is an encoder interface | ||
| // which handles encoding proto.Message to sarama.ProducerMessage | ||
| type MessageEncoder interface { | ||
| Encode(proto.Message) ([]*sarama.ProducerMessage, error) |
There was a problem hiding this comment.
I think we can keep the interface generic the way it was intended, so we don't couple ourselves with the types of messages we're handling.
| Stop() | ||
| } | ||
|
|
||
| type producer struct { | ||
| notifsChan chan proto.Message | ||
| notifsChan chan *pb.SubscribeResponse |
There was a problem hiding this comment.
This is failing tests, please run make check
| ) | ||
|
|
||
| // MessageEncoder is an encoder interface | ||
| // which handles encoding proto.Message to sarama.ProducerMessage | ||
| type MessageEncoder interface { | ||
| Encode(proto.Message) ([]*sarama.ProducerMessage, error) | ||
| Encode(*pb.SubscribeResponse) ([]*sarama.ProducerMessage, error) |
change comments and format to honor golint and gofmt update "go get" link to use x/lint repo
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
=======================================
Coverage 76.17% 76.17%
=======================================
Files 12 12
Lines 957 957
=======================================
Hits 729 729
Misses 203 203
Partials 25 25Continue to review full report at Codecov.
|
| update := response.GetUpdate() | ||
| if update == nil { | ||
| return nil, UnhandledSubscribeResponseError{response: response} | ||
| } | ||
| updateMap, err := openconfig.NotificationToMap(e.dataset, update, | ||
| elasticsearch.EscapeFieldName) | ||
| updateMap, err := gnmi.NotificationToMap(update) |
There was a problem hiding this comment.
I don't know if you tested this but gnmi.NotificationToMap() isn't really a drop in replacement for openconfig.NotificationToMap(). Unlike the openconfig version, the gnmi version flattens the update so instead of:
updates: {
foo: {
bar: {
...
you have:
updates: {
foo/bar: ...
I think eventually we'll end up flattening the schema and consolidating it with what we have in our own backend, but in the meanwhile would you be up for keeping the format the same? If you prefer the format you implemented here keeping your own for is also an option.
There was a problem hiding this comment.
I don't know if you tested this but gnmi.NotificationToMap() isn't really a drop in replacement for openconfig.NotificationToMap().
No it isn't, and neither gnmi.Path is a drop in replacement for openconfig.Path. :-)
I think eventually we'll end up flattening the schema and consolidating it with what we have in our own backend, but in the meanwhile would you be up for keeping the format the same? If
@7AC may I ask why we really need to? I see cmd/ocsplunk uses the flattened path value of gnmi.NotificationToMap() directly for writing events, which I believe is because that's how it's supposed to be. Doesn't the same go for kafka agent?
There was a problem hiding this comment.
The initial use case for ockafka was for Kafka -> Logstash -> Elasticsearch -> Kibana. Having an entire device as a single JSON document makes a really nice representation in Elasticsearch, and that's what you see implemented here (before your changes). It does run into some limitations when it comes to scaling (max number of fields, fields changing type, etc). We have some changes coming to ockafka that work around these limitations which will cause schema changes. As a result I'm a bit reluctant to make an additional schema change here. So I think we can go two routes:
- hold off merging this and reconcile with the changes on our end once they come in
- change the PR so the schema we output looks the same (basically cmd/ockafka: Use gNMI client instead of openconfig. #22 (comment))
What do you think? By the way sorry for the delayed response it was a long weekend in the US.
There was a problem hiding this comment.
@7AC many thanks for the clarification. Now I see how you plan to have it grow, the route 1 sounds natural to me.
hold off merging this and reconcile with the changes on our end once they come in
Hope your revision for the work around will go smoothly. And also hope you enjoyed the holidays!
|
Can you mention #23 in the commit? |
No description provided.