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

Logs not structured when both --structured-logs and --quiet are used together #1738

Closed
kaiburjack opened this issue Apr 7, 2023 · 2 comments · Fixed by #1750
Closed

Logs not structured when both --structured-logs and --quiet are used together #1738

kaiburjack opened this issue Apr 7, 2023 · 2 comments · Fixed by #1750
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@kaiburjack
Copy link

Bug Description

In an attempt to identify log entries that are actual errors and reduce noise from log entries which aren't, but are detected as such by the default fluentbit log agent (and thus by GCP Logging Explorer) because they are written text form to the POSIX STDERR stream, we tried to configure the Google Cloud Sql Proxy using both:

  • --structured-logs (in order to help fluentbit/stackdriver to detect actual errors and not flag text output as errors because it was printed to STDERR)
  • --quiet (because all those "certificate expires, getting a new one..." or "client opened a new connection..." wasn't giving us much information and we tried to lower the overall logging storage costs.)

However, everytime the cloud sql proxy container gets terminated (because of a simple scaledown of the pod it is living in), we noticed the unstructured log output (to STDERR): SIGTERM signal received. Shutting down... which is coming from

cmd.logger.Errorf("SIGTERM signal received. Shutting down...")

First: This is arguably not an error.
Second: This is also not structured, because whenever --structured-logs is used together with --quiet, the structured logger is again replaced by the NewStdLogger, as can be seen in this code section:

cloud-sql-proxy/cmd/root.go

Lines 344 to 349 in 6284a69

if c.conf.StructuredLogs {
c.logger, c.cleanup = log.NewStructuredLogger()
}
if c.conf.Quiet {
c.logger = log.NewStdLogger(io.Discard, os.Stderr)
}

It would be nice, if both:

  • SIGTERM signal received. Shutting down... would not be logged/treated as an error (maybe a warning)
  • using structured-logs together with quiet would not disable structured logging

Example code (or command)

No response

Stacktrace

No response

Steps to reproduce?

Run cloudsql proxy with both --structured-logs and --quiet and issue a SIGTERM signal (as is used in Kubernetes) to stop it.

Environment

  1. OS type and version: Standard GKE 1.25 Linux OS with containerd
  2. Cloud SQL Proxy version (./cloud-sql-proxy --version): 2.1.2
  3. Proxy invocation command (for example, ./cloud-sql-proxy --enable_iam_login --dir /path/to/dir INSTANCE_CONNECTION_NAME):
        - name: cloud-sql-proxy
          image: eu.gcr.io/cloud-sql-connectors/cloud-sql-proxy:2.1.2-alpine
          args:
            - "--private-ip"
            - "--quiet"
            - "--structured-logs"
            - "--health-check"
            - "--http-port=9090"
            - "--max-sigterm-delay=15s"
            - "--port=5432"
            - "..."

Additional Details

No response

@kaiburjack kaiburjack added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Apr 7, 2023
@kaiburjack kaiburjack changed the title Logs and not structured when both --structured-logs and --quiet are used together Logs not structured when both --structured-logs and --quiet are used together Apr 7, 2023
@enocom enocom added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Apr 7, 2023
@enocom
Copy link
Member

enocom commented Apr 7, 2023

Thanks for the report @kaiburjack. We'll work this into our next iteration and get it fixed soon.

@jackwotherspoon
Copy link
Collaborator

@kaiburjack This change will be reflected when v2.2.0 is released (which will be sometime tomorrow).

Thanks again for raising this! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants