-
Notifications
You must be signed in to change notification settings - Fork 25
Add common spec and run validation against it #23
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
Conversation
Do not allow to configure format for @timestamp.
* fix: timestamp format to ISO8601 * add tests for spec validation implements elastic#19
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.
Mostly LGTM, just a couple of small things
logger_test.go
Outdated
if field.Required { // all required fields must be present in the log line | ||
require.Contains(t, tw.m, name) | ||
require.NotNil(t, tw.m[name]) | ||
} |
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.
This will never fail, because we're ranging over tw.m
rather than spec.V1.Fields
. We should swap that around.
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.
good catch - updated
This PR copies the common spec for ECS loggers and runs validations for the fields that are already supported.
The spec itself is just manually copied over, as according to https://github.com/elastic/observability-robots/issues/313 the spec should automatically be synced when changed.
While on it, a bug was found in the
timestamp
format, where the timestamp can be changed from the defaultISO8601
format to any other format. With this fix the timestamp will always beISO8601
for the ECS formatter.closes #19