Add StrictMode with event validation#90
Add StrictMode with event validation#90rdner merged 2 commits intoelastic:mainfrom rdner:event-validation
StrictMode with event validation#90Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
|
||
| _ "github.com/elastic/elastic-agent-libs/logp/configure" | ||
| "github.com/elastic/elastic-agent-shipper/server" | ||
| "github.com/elastic/elastic-agent-shipper/controller" |
There was a problem hiding this comment.
I moved controller-related stuff to a separate package to resolve the dependency loop with ./config/config.go.
In `StrictMode` required fields are: * Timestamp * Datastream.Namespace * Datastream.Dataset * Datastream.Type * Source.InputId
| Metadata: sampleValues, | ||
| Fields: sampleValues, | ||
| }, | ||
| expectedMsg: "timestamp: proto:\u00a0invalid nil Timestamp", |
There was a problem hiding this comment.
proto:\u00a0invalid
Is the \u00a0 intentional here?
There was a problem hiding this comment.
well, this what the gRPC code returns, I had to include it.
There was a problem hiding this comment.
Ah, you could use https://pkg.go.dev/google.golang.org/grpc/status#Convert to convert the error to a status object with both the code and message parsed out to avoid this.
There was a problem hiding this comment.
Ah, you could use https://pkg.go.dev/google.golang.org/grpc/status#Convert to convert the error to a status object with both the code and message parsed out to avoid this.
@cmacknz By code I meant "source code", I don't know why but sometimes it's returned with this character, sometimes it is not. The weird thing is that it does not seem to be flaky when I run tests.
There was a problem hiding this comment.
Ah I misinterpreted. Convert or FromError are fine, I missed that it was already used.
The Unicode character is a non-breaking space and it is coming from this code that appears to be deliberately randomizing the returned error string: https://github.com/protocolbuffers/protobuf-go/blob/fb0abd915897428ccfdd6b03b48ad8219751ee54/internal/errors/errors.go#L30
In the timestamppb unit tests they disable the randomization, but the Disable function comes from an internal package: https://github.com/protocolbuffers/protobuf-go/blob/fb0abd915897428ccfdd6b03b48ad8219751ee54/types/known/timestamppb/timestamp_test.go#L22
They also use a wrapper type to avoid direct equality comparisons which might be what we want to do here: https://github.com/protocolbuffers/protobuf-go/blob/fb0abd915897428ccfdd6b03b48ad8219751ee54/types/known/timestamppb/timestamp_test.go#L104
We could also not care about the exact error as long as some error is returned since it is the return value from a method of the timestamppb type we didn't implement ourselves.
All of this seems a bit silly, they are discouraging direct error comparisons but in a highly non-obvious way if I have to read the implementation to figure out what is happening.
There was a problem hiding this comment.
The test also seems to pass reliably so this bit of magic in the proto package might not even be doing its job properly in that the error string isn't actually randomized, just unexpected. 🤷
There was a problem hiding this comment.
I'll change the test check in the following PR (which is based on this and in the draft mode for now).
Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
cmacknz
left a comment
There was a problem hiding this comment.
LGTM, would be nice to parse the returned errors in the tests with https://pkg.go.dev/google.golang.org/grpc/status#Convert but not blocking my approval on that.
|
/test |
|
/test |
|
/test |
In
StrictModerequired fields are:Closes #79