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

Normalize the Spectrum log messages #2279

Merged
merged 1 commit into from
May 12, 2021

Conversation

orpiske
Copy link
Contributor

@orpiske orpiske commented May 11, 2021

Fixes GH issue #2276

Release Note

Normalize Spectrum build messages

@orpiske
Copy link
Contributor Author

orpiske commented May 11, 2021

This is how they look like

camel-k-spectrum

@orpiske orpiske force-pushed the normalize-spectrum-logs branch 2 times, most recently from a28241a to a128e22 Compare May 11, 2021 09:33
Copy link
Member

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

In order to prevent regression from unstructured log statements sneaking in, it would be useful to add an e2e test, that unmarshalls the operator logs.

An option could be added to:

if err != nil {

in order to error the test in that case.

@orpiske
Copy link
Contributor Author

orpiske commented May 11, 2021

In order to prevent regression from unstructured log statements sneaking in, it would be useful to add an e2e test, that unmarshalls the operator logs.

An option could be added to:

if err != nil {

in order to error the test in that case.

Thanks. That's a good idea. I'll implement that.

@orpiske
Copy link
Contributor Author

orpiske commented May 12, 2021

I am working on the test case for this. It took me a while to get Kind running locally.

@orpiske
Copy link
Contributor Author

orpiske commented May 12, 2021

@astefanutti I think we'll have to leave the integration test for a separate PR. We have other log messages that are non-structured. They are not printed by our code directly, instead they are printed by dependencies (i.e.: leader election from k8s is one of the offending code). So we'll have to review those before introducing a test for this.

If you are OK with that, my suggestion is to go as it is and I'll open another issue for myself related to the remaining non-structured bits ... and that one will contain a test for the structured logging.

wdyt?

@astefanutti
Copy link
Member

@orpiske thanks a lot for the update!

Indeed, structured logging is still in alpha in Kubernetes 1.19:
https://kubernetes.io/blog/2020/09/04/kubernetes-1-19-introducing-structured-logs/. It's also not really clear how to activate structured logging in client-go (which hosts the leader election code that controller-runtime calls), maybe klog.SetLogger() can be used?

I agree with your suggestion to merge this, and create a follow up issue. In addition to fixing the Spectrum case, this PR has also helped identifying the remaining bits 👍🏼.

@orpiske
Copy link
Contributor Author

orpiske commented May 12, 2021

@orpiske thanks a lot for the update!

Indeed, structured logging is still in alpha in Kubernetes 1.19:
https://kubernetes.io/blog/2020/09/04/kubernetes-1-19-introducing-structured-logs/. It's also not really clear how to activate structured logging in client-go (which hosts the leader election code that controller-runtime calls), maybe klog.SetLogger() can be used?

I agree with your suggestion to merge this, and create a follow up issue. In addition to fixing the Spectrum case, this PR has also helped identifying the remaining bits 👍🏼.

Thanks! I opened #2286 and I will explore that one next.

@orpiske orpiske merged commit 9fccb36 into apache:main May 12, 2021
@nicolaferraro nicolaferraro mentioned this pull request Jul 2, 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.

None yet

3 participants