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

Fix mixed log using string concat #200

Merged
merged 2 commits into from
Sep 22, 2021

Conversation

aureplop
Copy link
Contributor

@aureplop aureplop commented Sep 14, 2021

(Should) fix #195.

I did not test it yet this version where we had the issues but it would be nice to check that before merging it, unless reviewers of this are already confident it will do the trick to fix the issue mentioned above.

Concatenation with "+" may not be the most performant thing but anyway this default logging function is very rarely used. I only saw the log in #195 using this format.

Also, are we ok that the log message will still be flushed to stderr correctly, even if it's not using std::endl?

Finally, I'm not sure to understand why the "build" step of CI is failing, but it also fails on other recent PR 🤔

@aureplop aureplop force-pushed the aureplop/fix-log-buffering branch from a4d2e8b to 28320c3 Compare September 14, 2021 13:37
@cgilmour
Copy link
Contributor

Thanks @aureplop ! Are you able to build the plugin and test it?

I'll do a separate PR to clear up the CI issues.

@cgilmour cgilmour mentioned this pull request Sep 15, 2021
@aureplop
Copy link
Contributor Author

Are you able to build the plugin and test it?

I will give it a try on Friday. Stay tuned 🥁

@aureplop
Copy link
Contributor Author

And so I succeed to test it locally. And this is working as expected 🎉

With current master, using a config with worker_processes 4, I was reproducing the behavior described in #195 with the startup logs as expected. It only happens from time to time so I was running some nginx -s reload and I had some log formatting issues maybe 1 over 5 times.

On the branch on this PR, with the same setup, I did not get any issues even with a hundred of reloads.

I believe an (integration) test could be added, but it may be a bit boilerplate due to randomness (we could run the reload and check 100 times to mitigate) and it needs a change to the test config (have multiple worker_processes, currently it uses the default value of 1). Not sure this is actually worth it.

Do you need something more from me to merge this PR @cgilmour? Thanks!

@aureplop aureplop marked this pull request as ready for review September 18, 2021 12:06
@cgilmour
Copy link
Contributor

Right, it's hard to be certain if this resolves the problem or just makes it less likely to occur.
I don't think additional tests are needed, and for the most part, it's a cosmetic issue.
A future change will probably only make this message get written when a debug flag or environment variable is set.

Thanks for performing the test to confirm it resolved the issue in your case!

Copy link
Contributor

@cgilmour cgilmour left a comment

Choose a reason for hiding this comment

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

Looks good and seems to resolve the reported problem!

@cgilmour cgilmour merged commit 0cb27e4 into DataDog:master Sep 22, 2021
@cgilmour
Copy link
Contributor

Merged, thanks @aureplop
Is there urgency for a release that contains this change?

@aureplop aureplop deleted the aureplop/fix-log-buffering branch September 22, 2021 15:32
@aureplop
Copy link
Contributor Author

Thanks! No emergency to release, we are using a env var to disable the startup logs causing parsing issues and this is good enough for now ;)

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.

NGINX logs using the default logging function are badly mixed with other logs, leading to parsing errors
2 participants