Use OpenTelemetry Semantic Conventions by satabin · Pull Request #1255 · typelevel/skunk

satabin · 2026-01-23T17:21:00Z

This is an attempt at #1127 and an opportunity to align the existing traces with the ones defined in semantic conventions.

The current PR:

improves existing spans by adding as many semantic convention attributes as possible
includes available details in attributes in case we receive a PostgresErrorException
creates client spans with custom finalizer to implement the above point
records the operation duration on each exchange, adding as many attributes as possible

There currently are some limitations with this implementation:

we do not have direct access to low-cardinality query summary, or I did not find how to get access to it
span names are still the same generic names as before (e.g. query or execute)
database name or query operation is not easily available on a query and is not added to the attributes
- if there is a way to get these pieces of information easily that would be great, maybe I missed something
this PR does not add the metrics which are still in development (e.g. the db.client.response.returned_rows or connection pool metrics are not added)
- this could be added in a follow-up PR maybe
it breaks the source and binary compatibility as the Histogram has to be propagated, I think it is not an issue, correct me if I'm wrong.

mpilquist · 2026-01-24T16:42:26Z

modules/core/shared/src/main/scala/util/Otel.scala

+object Otel {
+
+  // TODO the current snapshot used does not have the new `From` instance,
+  // and I cannot update the otel4s dependency since skunk depends on SN 0.5


Is this still true with the snapshot we just upgraded to last week?

I rebased on main yesterday and it was still the case. Maybe we need another snapshot @iRevive?

Here are updated snapshots:

lazy val otel4sVersion = "0.16-83b6f7b-SNAPSHOT" lazy val otel4sSdkVersion = "0.17-c767f1d-SNAPSHOT"

mpilquist · 2026-01-24T16:47:11Z

modules/tests/shared/src/main/scala/ffstest/FFramework.scala


 trait FTest extends CatsEffectSuite with FTestPlatform {

+  implicit val meter: Meter[IO] = Meter.noop


Can we tie this in with the other test machinery (withinSpan, tracedTest, etc)

I didn't have time to push this change, I will do it in a follow-up PR, then.

Yep no worries

mpilquist · 2026-01-24T16:52:27Z

modules/core/shared/src/main/scala/Session.scala

   * @param parseCacheSize        size of the pool-level cache for parsing statements; defaults to 2048
   */
-  final class Builder[F[_]: Temporal: Network: Console] private (
+  final class Builder[F[_]: Temporal: Meter: Network: Console] private (


Curious if we should just take a Tracer here too and drop support for explicitly passing a Tracer? Or alternatively, should we support explicitly passing a Meter using a similar pattern? Explicit passing was added back when tracing was done via Natchez: #650

I think the implicit Tracer is the more idiomatic way of doing it in otel4s. This could be done in a follow-up PR?

Yep for sure

It would be nice to use MeterProvider so you can explicitly define and control the instrumentation scope. Then, you can create a Meter inside the pooled / single:

val meter: F[Meter] = MeterProvider[F].tracer("skunk").withVersion(BuildInfo.version).get

OpenTelemetry generally recommends that libraries own their instrumentation scopes.
We follow a similar approach in http4s-otel4s-middleware or fs2-grpc-otel4s.

If possible, I would do the same with Tracer.

If users don't need tracing or metering capabilities, they can use given MeterProvider[F] = MeterProvider.noop.

Happy to do that in a follow up PR. Same for TraceProvider, right?

If that works for you, We can integrate the metrics and then I can do a cleanup PR with TracerProvider and MeterProvider, and get rid of the explicit Tracer API. TO avoid blowing up the size of this PR. I'd like also to follow-up with metrics for the pool, as per the semantic convention.

satabin added 5 commits January 23, 2026 17:41

Use semantic convention span attributes

2a7aa52

Add basic metrics

caa70a4

Remove unused import

8533a0c

Cleanup unnecessary public histogram field

42c1695

Update documentation with noop meters

b1e5558

satabin force-pushed the telemetry-semconv branch from 76cf10c to b1e5558 Compare January 23, 2026 17:45

Rename the Tracing section into a more general Telemetry one

c52e9fb

satabin force-pushed the telemetry-semconv branch from 1fb5ce0 to c52e9fb Compare January 23, 2026 17:56

Instantiate meter only once

248b3a5

mpilquist reviewed Jan 24, 2026

View reviewed changes

Bump to latest snapshots

8118b35

mpilquist merged commit d42e571 into typelevel:main Jan 25, 2026
10 checks passed


		trait FTest extends CatsEffectSuite with FTestPlatform {

		implicit val meter: Meter[IO] = Meter.noop

Uh oh!

Conversation

satabin commented Jan 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iRevive Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

iRevive Jan 25, 2026 •

edited

Loading