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

CRI log parser improvements #228

Closed
jreyeshdez opened this issue Jan 26, 2018 · 3 comments
Closed

CRI log parser improvements #228

jreyeshdez opened this issue Jan 26, 2018 · 3 comments

Comments

@jreyeshdez
Copy link
Contributor

The function parseCRILog might have some room to introduce some changes to make it more performance as calling strings.Fields only once at the beginning of the method should be more efficient. I think it could even be more efficient if we use strings.SplitN as I think we do not need to split the whole message.

Likewise, it would be good to check the resulting array to make sure it has the correct or the expected size and avoid errors when accessing different fields, for example 2016-10-06T00:17:10.113242941Z stdout as an input would cause panic.

If we use strings.SplitN then we wouldn't need strings.Join.

@feiskyer
Copy link
Member

@Random-Liu What do you think about this?

@Random-Liu
Copy link
Contributor

Random-Liu commented Jan 29, 2018

The function parseCRILog might have some room to introduce some changes to make it more performance as calling strings.Fields only once at the beginning of the method should be more efficient. I think it could even be more efficient if we use strings.SplitN as I think we do not need to split the whole message.

That is util function used in the validation test, and performance shouldn't be a problem there. But feel free to send a PR to improve it. :) Thanks!

Likewise, it would be good to check the resulting array to make sure it has the correct or the expected size and avoid errors when accessing different fields, for example 2016-10-06T00:17:10.113242941Z stdout as an input would cause panic.

Agree.

@Random-Liu
Copy link
Contributor

I think we can close this one.

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

No branches or pull requests

3 participants