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

Add some Prometheus metrics: #501

Closed
wants to merge 3 commits into from

Conversation

tomwilkie
Copy link

@tomwilkie tomwilkie commented Oct 14, 2020

Change Description

Add some Prometheus metrics:

  • cloudsql_proxy_active_connections
  • cloudsql_proxy_max_connections
  • cloudsql_proxy_connections_total
  • cloudsql_proxy_connections_errors_total

Checklist

  • Make sure to open an issue as a
    bug/issue
    before writing your code! That way we can discuss the change, evaluate
    designs, and agree on the general idea.
  • Ensure the tests and linter pass
  • Appropriate documentation is updated (if necessary)

Relevant issues:

- cloudsql_proxy_active_connections
- cloudsql_proxy_max_connections
- cloudsql_proxy_connections_total
- cloudsql_proxy_connections_errors_total

Signed-off-by: Tom Wilkie <[email protected]>
@kurtisvg
Copy link
Contributor

kurtisvg commented Oct 14, 2020

2 quick high level thoughts:

  1. I'd much rather target Opencensus and be able to support both Prometheus and Stackdriver metrics
  2. We probably (at least initially) mark the flag as experimental - particularly because I would ideally like to be able to provide more instance specific connections, and don't want to have to do a major version bump if that effects the metric surface.

@Carrotman42
Copy link
Contributor

Just so that we are clear about what the impact of adding this dependency is, do you mind reporting the before/after size of the binary?

@tomwilkie
Copy link
Author

I'd much rather target Opencensus and be able to support both Prometheus and Stackdriver metrics

I thought Opencensus was deprecated and Opentelemetry wasn't ready yet... Either way not super familiar with either. I think there are plenty of ways to get Prometheus metrics into stackdriver.

We probably (at least initially) mark the flag as experimental - particularly because I would ideally like to be able to provide more instance specific connections, and don't want to have to do a major version bump if that effects the metric surface.

Done!

Just so that we are clear about what the impact of adding this dependency is, do you mind reporting the before/after size of the binary?

twilkie@MacBook-Pro cloudsql-proxy % git checkout  master 
twilkie@MacBook-Pro cloudsql-proxy % GOOS=linux go build ./cmd/cloud_sql_proxy
twilkie@MacBook-Pro cloudsql-proxy % ls -l cloud_sql_proxy                   
-rwxr-xr-x  1 twilkie  staff  16391258 15 Oct 09:50 cloud_sql_proxy
twilkie@MacBook-Pro cloudsql-proxy % git checkout  prometheus 
Switched to branch 'prometheus'
twilkie@MacBook-Pro cloudsql-proxy % GOOS=linux go build ./cmd/cloud_sql_proxy
twilkie@MacBook-Pro cloudsql-proxy % ls -l cloud_sql_proxy                    
-rwxr-xr-x  1 twilkie  staff  17930396 15 Oct 09:51 cloud_sql_proxy

@kurtisvg
Copy link
Contributor

I thought Opencensus was deprecated and Opentelemetry wasn't ready yet... Either way not super familiar with either. I think there are plenty of ways to get Prometheus metrics into stackdriver.

OpenCensus isn't deprecated, but most of the feature work is focused on OpenTelemetry. OpenTelemetry is currently in Beta and is supposed to GA sometime before the end of the year.

You can export Prometheus metrics to Stackdriver, but requires you to collect them in a prometheus server and export them. Using OpenCensus means we could provide users an option to specify an exporter to either Prometheus or Stackdriver (or one of the other supported exporters).

@withnale
Copy link

Is this PR dead now? It was going to provide really useful visibility

@enocom
Copy link
Member

enocom commented Feb 10, 2021

As mentioned in #600, we're working on some major structural improvements in addition to waiting for OpenTelemetry to go GA. Once those are done, we're going to circle back and add first-class support for metrics and tracing to the proxy. For now, I'd like to leave this PR open for reference in case others need Prometheus support with the caveat that we're not going to merge it.

Base automatically changed from master to main February 17, 2021 18:05
@enocom enocom added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 10, 2021
@enocom
Copy link
Member

enocom commented Jun 13, 2022

As of #1215, we have support for Prometheus metrics in the v2 branch. We're working on a migration guide and making v2 the main branch, but until then I'm going to close this.

@enocom enocom closed this Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for metrics / tracing
6 participants