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

Initial implementation for ECS zerolog #1

Merged
merged 5 commits into from
Jun 8, 2021
Merged

Initial implementation for ECS zerolog #1

merged 5 commits into from
Jun 8, 2021

Conversation

michel-laterman
Copy link
Contributor

Add an ECS compliant zerolog constructor.
The constructor function alters zerolog package variables such as MessageFieldName for formatting.
Most of the implementation was inspired by previous work for the ECS zap/logrus modules.

example_test.go Outdated
// {"log.level":"error","ecs.version":"1.6.0","error.message":"something bad happened","message":"An error has occured"}
}

// FIXME: Needs discussion on stack_trace, may not want to include as an example.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enabling the stack traces places absolute paths in the output; this would make the example not very portable. I can easily add a unit test that checks if the key is set in logging_test.go if we want this explicitly tested

Copy link

Choose a reason for hiding this comment

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

adding a unit test for the key sounds fine

package spec

import (
"embed"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cleaned the init login by using embed.FS instead of opening/reading the spec file

Copy link

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for putting this up!

I'll put up a couple of follow up issues related to documentation, CI etc.

Str("file.name", file).
Int("file.line", line),
)
}
Copy link

Choose a reason for hiding this comment

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

Could you add a test for this?

example_test.go Outdated
// {"log.level":"error","ecs.version":"1.6.0","error.message":"something bad happened","message":"An error has occured"}
}

// FIXME: Needs discussion on stack_trace, may not want to include as an example.
Copy link

Choose a reason for hiding this comment

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

adding a unit test for the key sounds fine

"os"
"time"

"go.elastic.co/ecszerolog"
Copy link

Choose a reason for hiding this comment

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

Are you taking care of getting this set up? (Or is it already set up?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has not been setup yet

Copy link

Choose a reason for hiding this comment

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

The repo is still private, but we should turn it public once this PR is merged and the namespace is setup.

@simitt Can you help here?

Copy link

Choose a reason for hiding this comment

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

It's basically creating a PR similar to https://github.com/elastic/infra/pull/19734 and merging once the repo is public.

@@ -0,0 +1,143 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

After merging the initial implementation, we should enable the sync for the spec for this repo, too.

See also elastic/ecs-logging#47

Copy link

Choose a reason for hiding this comment

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

Good point. @michel-laterman Please open an issue in the observabolity-robots repository to get help.


"go.elastic.co/ecszerolog"

"github.com/pkg/errors"
Copy link

@urso urso Jun 7, 2021

Choose a reason for hiding this comment

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

We should not use that package in new code if we can. It is in 'maintenance' mode, but in general development/support for this packages has ceased.

Alternative to errors.Wrap is fmt.Errorf("...%w", ..., err). This will capture the error parameter and produce a wrapping error.

Looks like you imported that one by accident. Did you mean "errors" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, but the pkg/errors module comes with stack tracing, do we want to support that?

Copy link

Choose a reason for hiding this comment

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

It's just used for tests here. Might be nice to have a test that also include stack traces ;)

@michel-laterman michel-laterman merged commit d0099b2 into elastic:main Jun 8, 2021
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.

[Go] zerolog
4 participants