-
Notifications
You must be signed in to change notification settings - Fork 21
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
Adds synchronous and asynchronous OpenTelemetry metrics #43
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this comprehensive MR 🥳
In general it looks good, just a couple nits.
meter.go
Outdated
pgxPoolAcquireCount, | ||
metric.WithDescription("Cumulative count of successful acquires from the pool."), | ||
); err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we expand the error messages a bit?
return err | |
return fmt.Errorf("create %s: %w", pgxPoolAcquireCount, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! Updated
|
||
var ( | ||
pgxPoolAcquireCount = "pgxpool.acquires" | ||
pgxPoolAcquireDuration = "pgxpool.acquire_duration" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so familiar with the best practices around metric naming here, but would it make sense to add the base unit?
pgxPoolAcquireDuration = "pgxpool.acquire_duration" | |
pgxPoolAcquireDuration = "pgxpool.acquire_duration_seconds" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as each metric specifies their unit upon creation, we shouldn't need to include them in the metric name ourselves.
https://opentelemetry.io/docs/specs/semconv/general/metrics/#units
To this end, I did realize I missed specifying the unit in the async. metric pgxPoolAcquireDuration
in meter.go
. This has now been fixed.
FWIW as well-- Depending on users' observability stack and its own configuration, it is possible for the metric unit to be added to the metric name later on.
Using Prometheus as an example, it will append _milliseconds_bucket
to the metric name db_client_operation_duration
.
Line 136 in f84cbc7
metric.WithUnit("ms"), |
Problem Statement:
otelpgx
, as of today, is only responsible for tracing observability.Clients of
pgx
andotelpgx
are forced to manage their own metric observability which can lead to:pgx
because of implementation redundancyCurrently, PR #18 exists to add PGX Stats as Asynchronous Observable OpenTelemetry metrics.
This PR, while also instruments the same asynchronous metrics, aims to do so with a few changes and alongside additional features:
This PR also implicitly resolves issue #40.
PR #18 seems to have left off awaiting information for how to have OTLP metrics scraped by Prometheus.
Those instructions, at least to me, seem outside the scope of
otelpgx
since that is more infrastructure-setup specifics, which OpenTelemetry-ecosystem tends to be agnostic to, and could become a chore to maintain long-term.Because of that, I haven't included any specifics in the README as part of this PR; though if felt strongly otherwise, I can of course do so.
In my own environment (Application -> Collector <- Prometheus), I am able to verify metrics being reported.
See attached screenshots:
Screenshots