System metrics setup #1041
System metrics setup #1041
Conversation
panavenue force-pushed the system_metrics_setup branch from 8747506 to 1f835d8 Compare panavenue force-pushed the system_metrics_setup branch from 1f835d8 to ad4a6e6 Compare 
enocom left a comment
enocom left a comment There was a problem hiding this comment.
Glad to see the AlloyDB implementation was helpful. Left a couple of comments.
| func (d *Dialer) connectionInfoCache( | ||
| ctx context.Context, cn instance.ConnName, useIAMAuthN *bool, | ||
| ) (*monitoredCache, error) { | ||
| ) (*monitoredCache, bool, error) { |
There was a problem hiding this comment.
What is the purpose for the new "ok" result parameter? It looks like it is not used.
There was a problem hiding this comment.
I see now it is "cache hit" Add this to the method comment.
| | ||
| // DefaultExportInterval is the interval that the metric exporter runs. It | ||
| // should always be 60s. This value is exposed as a var to faciliate testing. | ||
| var DefaultExportInterval = 60 * time.Second |
There was a problem hiding this comment.
This should probably be a const, right?
| | ||
| // WithApplicationName returns an Option that sets the Application Name. | ||
| // This is used to identify the application in metrics. | ||
| func WithApplicationName(name string) Option { |
There was a problem hiding this comment.
WithApplicationID() now, I think.
| type Config struct { | ||
| // Enabled specifies whether the metrics should be enabled. | ||
| Enabled bool | ||
| // Version is the version of the alloydbconn.Dialer. |
| ConnectorVersion: versionString, | ||
| DatabaseEngineType: "DB-Engine-Type-Testing", // TODO: detect database engine type | ||
| } | ||
| mr := tel.NewMetricRecorder(ctx, d.logger, d.mClient, cfg) |
There was a problem hiding this comment.
What if mClient is not set?
| | ||
| // Resolve the instance connection name to a ConnName struct. | ||
| // Note: icn may be a domain name that resolves to an instance connection name. | ||
| cn, err := d.resolver.Resolve(ctx, icn) |
There was a problem hiding this comment.
We should count DNS lookup successes and errors. Does this make more sense as a label on our connect count metric, or is DNS resolution + status its own separate counter.
| func (d *Dialer) connectionInfoCache( | ||
| ctx context.Context, cn instance.ConnName, useIAMAuthN *bool, | ||
| ) (*monitoredCache, error) { | ||
| ) (*monitoredCache, bool, error) { |
There was a problem hiding this comment.
I see now it is "cache hit" Add this to the method comment.
| authCredentials *auth.Credentials | ||
| iamLoginTokenProvider auth.TokenProvider | ||
| useragents []string | ||
| applicationName string |
There was a problem hiding this comment.
applicationID with a TODO to set this by default using GCE VM Name, GKE pod name, Serverless app instance name (whatever that is).
3 participants
Description - This pull request introduces a comprehensive system for collecting and exporting client-side metrics from the Cloud SQL Go Connector, providing valuable insights into the connector's performance and behavior. The key changes include the integration of OpenTelemetry for metrics collection and the introduction of a dedicated telemetry package.
telwith new metrics -