[awsfirehosereceiver] Add auto type #36708
Conversation
|
Referencing #35988 (comment), @Aneurysm9 , does this approach look better? We check the bytes, we don't do multiple unmarshalls and we are capable now of not having to manually distinguish between these types. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
| @@ -159,13 +158,6 @@ func TestFirehoseRequest(t *testing.T) { | |||
| wantStatusCode: http.StatusInternalServerError, | |||
| wantErr: firehoseConsumerErr, | |||
| }, | |||
| "WithCorruptBase64Records": { | |||
There was a problem hiding this comment.
Sorry, my first reply to this was wrong. Before this PR, if there was a wrong record, then everything else would fail. Now this does not happen anymore (see this line), since we have a continue statement instead of throwing an error.
I did this because we follow the same logic for all unmarshallers, and I wanted to keep it consistent here too.
I have added the test back again, but now with the OK status.
There was a problem hiding this comment.
Ah I see. I was thinking of proposing that unmarshallers stop ignoring errors - I'm not aware that other receivers that behave like this, but maybe just out of ignorance. At any rate, I'd prefer that we don't expand this behaviour. If it's not strictly necessary for this PR, can we revert to the original behaviour, and keep it for another PR?
There was a problem hiding this comment.
Yes, I have reverted it now.
receiver/awsfirehosereceiver/internal/unmarshaler/cwmetricstream/unmarshaler.go
Show resolved
Hide resolved
receiver/awsfirehosereceiver/internal/unmarshaler/cwlog/unmarshaler.go
Outdated
Show resolved
Hide resolved
Please see the issue #35988 (comment).
This new
record_typedoes the exact same thing as the other record types, but is capable of distinguishing between them:I have used the exact same tests for this record type as the ones that are present for the other record types so there isn't any doubt that the new record type works.