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

gcp/observability: filter logging from cloud ops endpoints calls #5765

Merged
merged 3 commits into from Nov 7, 2022

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Nov 3, 2022

The filtering of logging from the calls to cloud ops endpoints was missed in the new Observability config iteration. This adds it back. Decided not to info log in this scenario to avoid std out log spam.

RELEASE NOTES: N/A

@zasweq zasweq requested a review from dfawley November 3, 2022 17:31
@zasweq zasweq added this to the 1.51 Release milestone Nov 3, 2022
Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test that covers these methods and expects no logging?

You should be able to use the native gRPC API to make it easy to set the method without needing the protos: https://pkg.go.dev/google.golang.org/grpc#ClientConn.Invoke

@dfawley dfawley assigned zasweq and unassigned dfawley Nov 3, 2022
@zasweq
Copy link
Contributor Author

zasweq commented Nov 7, 2022

Added a unit test. Thanks.

@zasweq zasweq assigned dfawley and unassigned zasweq Nov 7, 2022
@dfawley dfawley changed the title Filter logging from cloud ops endpoints calls gcp/observability: filter logging from cloud ops endpoints calls Nov 7, 2022
dfawley
dfawley approved these changes Nov 7, 2022
// calls, as they should be filtered out.
fle.mu.Lock()
if len(fle.entries) != 0 {
fle.mu.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: use a defer instead to avoid the two unlocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dfawley dfawley assigned zasweq and unassigned dfawley Nov 7, 2022
@zasweq zasweq merged commit 72812fe into grpc:master Nov 7, 2022
11 checks passed
jronak pushed a commit to jronak/grpc-go that referenced this pull request Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants