-
Notifications
You must be signed in to change notification settings - Fork 253
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
observe: warn on unknown field while JSON decoding #962
Conversation
Opening as draft as it will conflict with #958 and also unsure whether logging a warning message is the way to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time to ship
cmd/observe/io_reader_observer.go
Outdated
request *observer.GetFlowsRequest | ||
allow filters.FilterFuncs | ||
deny filters.FilterFuncs | ||
discardUnknown bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this going to be set?
cmd/observe/io_reader_observer.go
Outdated
// the error might be that the JSON data contains an unknown field. | ||
// This can happen we attempting to decode flows generated from a | ||
// newer Hubble version than the CLI (having introduced a new | ||
// field). Retry parsing discarding unknown fields and see whether | ||
// the decoding is successful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there nothing in the error that we can check to make sure it was unknown field, rather than attempting to parse twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking the error from protobuf it's from
return d.newError(tok.Pos(), "unknown field %v", tok.RawString()) |
hubble/vendor/google.golang.org/protobuf/encoding/protojson/decode.go
Lines 96 to 101 in bb32734
// newError returns an error object with position info. | |
func (d decoder) newError(pos int, f string, x ...interface{}) error { | |
line, column := d.Position(pos) | |
head := fmt.Sprintf("(line %d:%d): ", line, column) | |
return errors.New(head+f, x...) | |
} |
So I see no way to errors.Is
as it isn't wrapped. Matching on the error message doesn't feel right either.
Before this patch, when parsing JSON flows and encountering an unknown field, the CLI would log a warning and skip the flow. Unknown fields is not uncommon however, e.g. when using the Hubble CLI against a more recent Hubble server version. This patch * emit the flow if successfully parsed while ignoring unknown fields, * improve the warning message by hinting at upgrading the Hubble CLI, * make it so the warning message is logged only once. Signed-off-by: Alexandre Perrin <[email protected]>
bb32734
to
c237c6c
Compare
This only seems to update the io reader observer, what about the gRPC observer client? |
Yes good question, so functionally the gRPC client already ignore unknown fields. If we think the warning would be a good idea too for gRPC let's do it. |
@kaworu Ahhh, I see. Perhaps you could update the PR title/commit message to reflect this is only when ingesting flows from a file/stdin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM.
Before this patch, when parsing JSON flows and encountering an unknown field, the CLI would log a warning and skip the flow.
Unknown fields is not uncommon however, e.g. when using the Hubble CLI against a more recent Hubble server version.
This patch