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 support for CRI logs format #6181

Merged
merged 4 commits into from Jan 26, 2018

Conversation

Projects
None yet
5 participants
@jreyeshdez
Contributor

jreyeshdez commented Jan 25, 2018

Adds support for CRI logs format, also created tests for specific CRI logs.
Autodiscovery by looking at first char and it will invoke CRI or Docker log parser depending on it.
Solution for #5630

Julian Reyes
Adds support for CRI logs format.
Created tests for specific CRI logs.
Autodiscovers by looking at first char and it will invoke CRI or Docker log parser depending on it.
Solution for #5630
@karmi

This comment has been minimized.

Member

karmi commented Jan 25, 2018

Hi @jreyeshdez, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@elasticmachine

This comment has been minimized.

elasticmachine commented Jan 25, 2018

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@exekias

Awesome contribution @jreyeshdez, thank you so much for taking the time! 🎉

I left some comments, let me know what do you think

PD:

Have you tested this code (or you plan to) on a running cluster?

return message, errors.Wrap(err, "parsing CRI timestamp")
}
stream := strings.Fields(log)[1]

This comment has been minimized.

@exekias

exekias Jan 25, 2018

Member

Calling strings.Fields only once should be more efficient, you can do it at the beginning of this method and use the resulting variable. It will probably be more efficient if you use SplitN, to as you don't want to split the whole message.

Also it would be nice to check the resulting array, to ensure it's size is correct, and avoid errors when accessing different fields

This comment has been minimized.

@jreyeshdez

jreyeshdez Jan 26, 2018

Contributor

Thanks for your comments @exekias !
I will use SplitN and check array len, if less than 3 then it'll be considered an invalid CRI log.

This comment has been minimized.

@jreyeshdez

jreyeshdez Jan 26, 2018

Contributor

I'm planning to test the code on a running cluster, I tried yesterday but had some issues while setting up the environment on my Windows system.

if p.stream != "all" && p.stream != line.Stream {
continue
if strings.HasPrefix(string(message.Content), "{") {
message, err = parseDockerJSONLog(message.Content, &dockerLine, message)

This comment has been minimized.

@exekias

exekias Jan 25, 2018

Member

Having that you are passing message anyway, you could reduce the number of params to 2 (&message and &dockerLine). Same applies to parseCRILog

This comment has been minimized.

@jreyeshdez

jreyeshdez Jan 26, 2018

Contributor

Agree! I'll reduce it to 2 params.

@@ -32,12 +32,28 @@ func TestDockerJSON(t *testing.T) {
stream: "all",
expectedError: true,
},
// Wrong CRI
{
input: [][]byte{[]byte(`{this is not JSON nor CRI`)},

This comment has been minimized.

@exekias

exekias Jan 25, 2018

Member

I'm guessing 2017-09-12T22:32:21.212861448Z stdout as input would cause a panic, it would be nice to add it to the list of tests :)

This comment has been minimized.

@jreyeshdez

jreyeshdez Jan 26, 2018

Contributor

Good catch! I added it to the test cases, its covered by checking array len.

if p.stream != "all" && p.stream != line.Stream {
continue
if strings.HasPrefix(string(message.Content), "{") {

This comment has been minimized.

@ruflin

ruflin Jan 26, 2018

Collaborator

I wonder if we should make this a config option instead of "guessing" it here? Are there potentially other formats which will be added?

This comment has been minimized.

@exekias

exekias Jan 26, 2018

Member

I don't think new formats will be added, CRI is the standard Kubernetes went for, the idea of this is to support both the standard and old Docker format.
I'm ok with autodetecting it as of today, we can change this in the future if we see the point

Adds support for CRI logs format, also created tests for specific CRI…
… logs.

Made changes as per @exekias comments.
- Use `strings.SplitN` instead of calling multiple times `strings.Fields`.
- Reduce the number of params being passed on to functions.
- Check expected length for CRI log format.
// Anything after stream.
logMessage := strings.Fields(log)[2:]
logMessage := log[2:]
msg.Timestamp = ts
msg.Stream = stream
msg.Log = []byte(strings.Join(logMessage, " "))

This comment has been minimized.

@exekias

exekias Jan 26, 2018

Member

You no longer need strings.Join here, this should be just log[2]

This comment has been minimized.

@jreyeshdez

jreyeshdez Jan 26, 2018

Contributor

True, no need for strings.Join , but you meant msg.Log = []byte(logMessage[0]) right ?

This comment has been minimized.

@exekias

exekias Jan 26, 2018

Member

yep, or remove logMessage variable and just do msg.Log = []byte(log[2])

@exekias

This comment has been minimized.

Member

exekias commented Jan 26, 2018

Thank you for the update, I left one more comment, please address it and I think we are ready to go 😺

Adds support for CRI logs format
Made changes as per @exekias comments.
- No longer need `strings.Join`.
@exekias

Waiting for green, thank you so much! 🌮

@exekias

This comment has been minimized.

Member

exekias commented Jan 26, 2018

ok to test

@exekias exekias merged commit 6023701 into elastic:master Jan 26, 2018

3 of 5 checks passed

beats-ci Build started sha1 is merged.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
CLA Commit author has signed the CLA
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment