Skip to content
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

Add received to Event #85

Merged
merged 8 commits into from
Jun 23, 2023
Merged

Add received to Event #85

merged 8 commits into from
Jun 23, 2023

Conversation

endorama
Copy link
Member

@endorama endorama commented Jun 23, 2023

As part of data ingestion one interesting element we want to keep track of is when a certain event has been received by the ingestion backend.

This timestamp is not user-controlled like @timestamp and can guarantee us a certain timestamp for event reception, the step before ingestion (which is registered as event.ingested).

This PR adds the event.received field to protobuf Event and modeljson.Event.
This field value will be empty in modeljson.Event if it's not set in protobuf Event.

Tests have been adjusted to account for the additional field.

@endorama endorama self-assigned this Jun 23, 2023
@endorama endorama requested a review from a team June 23, 2023 10:54
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

model/modelpb/event.pb.json_test.go Outdated Show resolved Hide resolved
Copy link
Member

@kruskall kruskall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Sorry about the go 1.20 issue 🤦

@endorama
Copy link
Member Author

endorama commented Jun 23, 2023

@kruskall No worries and thanks for catching the bad test!

endorama and others added 8 commits June 23, 2023 15:08
Signed-off-by: Edoardo Tenani <[email protected]>
Signed-off-by: Edoardo Tenani <[email protected]>
`Event.Received` weren't really tested by the current tests, due to the
use of `cmpopts.IgnoreUnexported`. I wrongly assumed that with that
option only unexported fields would be ignored but in this case the
result was a complete lack of verification on the time value.

This commimt removes `IgnoreUnexported` and add a custom comparer to
ensure time correspond.

Co-authored-by: kruskall <[email protected]>
I'm using go 1.20 locally for testing while CI is using go 1.19.10.
This version does not have Time.Compare.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants