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

suppress noisy logs #3917

Merged
merged 2 commits into from Dec 9, 2021
Merged

suppress noisy logs #3917

merged 2 commits into from Dec 9, 2021

Conversation

satya-dillikar
Copy link
Contributor

Description of the change

Kubeapps-api is using "httpGet probe" for livenessProbe & readinessProbe every 10secs.
values.yaml

  livenessProbe:
    enabled: true
    httpGet:
      path: /core/plugins/v1alpha1/configured-plugins
      port: 50051
    initialDelaySeconds: 60
    periodSeconds: 10

  readinessProbe:
    enabled: true
    httpGet:
      path: /core/plugins/v1alpha1/configured-plugins
      port: 50051
    initialDelaySeconds: 0
    periodSeconds: 10

This is causing noisy logs of probe endpoint
/kubeappsapis.core.plugins.v1alpha1.PluginsService/GetConfiguredPlugins

as mentioned in PR#3780

Solution:

Option-1:
We can probably use "GRPC Health Checking Protocol" as mentioned in health-checking-grpc-servers-on-kubernetes

Adding “grpc-health-probe” needs to implement standard health check "protocol" as in health-checking and bundle health check utility "tool" with the kubeapps-api binary.

But this still does not solve the noisy logging of RPC calls.

Option-2:
Suppress the endpoints in the grpc interceptor call.

The commit implements

  1. Suppress the noisy endpoints
  2. uses consistent logging format (same as klog)

Benefits

no noisy endpoints logs

Possible drawbacks

N/A

Applicable issues

  • fixes #

Additional information

supressLoggingOfEndpoints := []string{"GetConfiguredPlugins"}
var level klogv2.Level

// level=3 is default logging level
Copy link
Contributor

Choose a reason for hiding this comment

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

The default log level is 3 if the user hasn't set a -v option on the cli... don't we need to default to whatever the user has specified (which will be 3 if nothing was specified)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should complicate more by using the user settings (--v).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I confused this in my head with something it is not... I was thinking we were deciding here what the default log level would be for the app, but it's not, it's just the level that will be used for the interceptor. Carry on :)


// level=3 is default logging level
level = 3
for i := 0; i < len(supressLoggingOfEndpoints); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's useful having suppressLoggingOfEndpoints as a string slice if it's something configurable or we have more than one endpoint being hit for liveness/readiness... but otherwise, why not just do a single string comparison here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Today, we have only one endpoint to suppress. I guess as we test the code more, we might have a few more endpoints.

I kept it as a string slice so that In the future, we can add more names to suppress.
This will be a one-time change and I don't think we need any configurable option for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, fine either way.

// Serve is the root command that is run when no other sub-commands are present.
// It runs the gRPC service, registering the configured plugins.
func Serve(serveOpts core.ServeOptions) error {
// Create the grpc server and register the reflection server (for now, useful for discovery
// using grpcurl) or similar.
logRequest := CreateRequestLogger()
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to delete the CreateRequestLogger() function to, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We can delete the CreateRequestLogger() as it is not used.

I left it unused. If you want we can remove it.
Please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please... we generally remove any unused code (it's just more code to maintain).

@absoludity
Copy link
Contributor

Thanks Satya! Just a couple of thoughts - otherwise +1

supressLoggingOfEndpoints := []string{"GetConfiguredPlugins"}
var level klogv2.Level

// level=3 is default logging level
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I confused this in my head with something it is not... I was thinking we were deciding here what the default log level would be for the app, but it's not, it's just the level that will be used for the interceptor. Carry on :)


// level=3 is default logging level
level = 3
for i := 0; i < len(supressLoggingOfEndpoints); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, fine either way.

// Serve is the root command that is run when no other sub-commands are present.
// It runs the gRPC service, registering the configured plugins.
func Serve(serveOpts core.ServeOptions) error {
// Create the grpc server and register the reflection server (for now, useful for discovery
// using grpcurl) or similar.
logRequest := CreateRequestLogger()
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please... we generally remove any unused code (it's just more code to maintain).

@satya-dillikar satya-dillikar merged commit 1591afc into vmware-tanzu:master Dec 9, 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

2 participants