Skip to content

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

Merged
merged 4 commits into from
Jan 13, 2021

Conversation

simitt
Copy link
Contributor

@simitt simitt commented Dec 28, 2020

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 default ISO8601 format to any other format. With this fix the timestamp will always be ISO8601 for the ECS formatter.

closes #19

Do not allow to configure format for @timestamp.
* fix: timestamp format to ISO8601
* add tests for spec validation

implements elastic#19
@simitt simitt added the review label Dec 28, 2020
@simitt simitt requested a review from axw December 28, 2020 21:09
@apmmachine
Copy link
Contributor

apmmachine commented Dec 28, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #23 updated

  • Start Time: 2021-01-13T09:19:45.241+0000

  • Duration: 4 min 12 sec

Test stats 🧪

Test Results
Failed 0
Passed 23
Skipped 4
Total 27

@simitt simitt changed the title Add commone spec and run validation against it Add common spec and run validation against it Dec 28, 2020
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.

Mostly LGTM, just a couple of small things

logger_test.go Outdated
Comment on lines 72 to 75
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])
}
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - updated

@simitt simitt requested a review from axw January 13, 2021 09:25
@simitt simitt merged commit 4479c79 into elastic:master Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 27] Validate logger against the spec
3 participants