XSAM / otelsql

OpenTelemetry instrumentation for database/sql
Apache License 2.0
300 stars 51 forks source link

Support for query specific attributes in `latency` histogram #333

Closed krantideep95 closed 1 month ago

krantideep95 commented 1 month ago

Problem Statement

Hi! thanks for the awesome pkg. It has been quite useful for instrumenting sql calls. We want to start leveraging metrics more instead of traces for creating our internal dashboards, given that traces may need to be sampled at some point.

There's a db.sql.latency histogram metric which would work. But it's missing a few attributes: database name, query operation name, table name etc. The semantic conventions for database metrics have the attributes documented but status is still "Experimental"

For traces, we have been able to reformat the span name using SpanNameFormatter and set more attributes using AttributesGetter. I was wondering if you'd open to supporting a similar AttributesGetter for metrics till status for metrics semconv is still Experimental? Happy to open a PR if this seems okay!

Proposed Solution

Similar to AttributesGetter for adding more attributes in span based on query context here: https://github.com/XSAM/otelsql/blob/de82710d7a7913cae38a261b656ba68b5b9e59b4/utils.go#L96-L98

same or another attributes getter for metrics to append attributes to metrics here https://github.com/XSAM/otelsql/blob/de82710d7a7913cae38a261b656ba68b5b9e59b4/utils.go#L67

Alternatives

otelsql could add all the attributes documented here by parsing queries, but:

Prior Art

Although not the same area, otelgrpc appends parsed rpc/context specific attributes to both spans & metrics: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/3fab5f5f20fb7f8ee7e33172fd129d336cd5640d/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L452-L462

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/3fab5f5f20fb7f8ee7e33172fd129d336cd5640d/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L336-L337

Additional Context

NA

XSAM commented 1 month ago

Hi, @krantideep95, thanks for raising this issue!

This looks like a good feature to have. We might need a new getter, InstrumentAttributesGetter. I prefer not to reuse AttributesGetter for metrics, as metrics storage is usually more sensitive than tracing storage in the cardinality of attributes.

Feel free to open a PR to implement this. You might need to extend recordMetric to include args []driver.NamedValue.