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(adapter): initialize klog into flags, to allow setting klog settings #775

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hermanbanken
Copy link

Fixes update to klog v2 in f3a96b5, which requires klog.InitFlags(nil) to be called before.

Fixes #640 when used with -v=1.

Fixes update to klog v2 in f3a96b5,
which requires klog.InitFlags(nil) to be called before.
@hermanbanken
Copy link
Author

@CatherineF-dev I think this small change would quickly resolve the issue #640. We're also seeing this in our prod environment. The 0.15 seemed to fix our http2: stream closed issues you referred to. Thanks! 🙏

@hermanbanken
Copy link
Author

cc @sophieliu15

@CatherineF-dev
Copy link
Contributor

Thanks! /lgtm

@CatherineF-dev
Copy link
Contributor

CatherineF-dev commented Sep 10, 2024

Could you help bump adapter version as well?

Similar to f072731

@hermanbanken
Copy link
Author

done!

@hermanbanken
Copy link
Author

I did notice KEP-2845 this morning. I'm not fully understanding the consequence/goals.

My main take-away was that the goal was to deprecate the requirement for klog in applications, while keeping -v and -vmodule, and keep log format similar to either JSON or text I0605 22:03:07.224378 3228948 logger.go:59] "Log using InfoS" key="value" format. I think we can safely keep klog, as long as we also keep -v and -vmodule exposed.

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.

Tracing custom-metrics-stackdriver-adapter trace logging enabled
2 participants