Skip to content

Conversation

@shikokuchuo
Copy link

@shikokuchuo shikokuchuo commented Oct 27, 2025

This PR implements basic OpenTelemetry instrumentation for DBI.

Abides by the otel semantic conventions for database spans as far as possible (with considerations for limitations of the R API, performance etc.).

The following is a screenshot of the spans created by running the examples for dbGetQuery(). This trace may also be examined interactively at this public link (30 day validity):
https://logfire-eu.pydantic.dev/public-trace/a3da0166-cf62-43de-b194-864bf3c9e33d?spanId=77e385b051f86076

Screenshot 2025-10-27 at 19 37 42

Implementation progress:

  • Implemented for: dbConnect/dbDisconnect, dbCreateTable/dbRemoveTable, dbGetQuery

Todo:

  • Extended coverage to: dbAppendTable, dbWriteTable/dbReadTable and all Arrow variants
  • Add tests
    - [ ] Add documentation (covered by news item + separate article for other packages)

I've assumed otel to be an 'imports' package for simplicity, but it shouldn't be a problem to move to 'suggests' if that's the preference.

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, lovely!

For what functions does it not make sense to implement telemetry? I guess dbQuote*(), what else?

Does this supersede https://github.com/r-dbi/dblog? Do you think a non-invasive approach like used there would be feasible here as well? What is the overhead if no listeners are active?

If we need to add here, a suggested package would be preferred.

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great first step!

@shikokuchuo
Copy link
Author

shikokuchuo commented Oct 28, 2025

For what functions does it not make sense to implement telemetry? I guess dbQuote*(), what else?

My preliminary thoughts are that we should instrument all 'full transactions', where we might be interested in the length of the spans. Otel expects all spans to be short-lived. Hence for example, we don't have a span that starts with dbConnect() and ends with dbDisconnect(), we have spans for each.

Does this supersede https://github.com/r-dbi/dblog? Do you think a non-invasive approach like used there would be feasible here as well? What is the overhead if no listeners are active?

Gabor has spent a lot of time to make the interface as user-friendly as possible. I think the idea that you can get instrumentation for free with no code changes, and can leave it on in production is a powerful proposition.

If not active, there is practically no overhead - the current main instrumentation function otel_local_active_span() is guarded by an early return based on a variable that will be cached (implemented in d6213f6). The design is so that arguments would remain unevaluated.

If we need to add here, a suggested package would be preferred.

Updated to suggests in d6213f6.

@shikokuchuo shikokuchuo marked this pull request as ready for review October 31, 2025 16:41
@shikokuchuo
Copy link
Author

I've updated this PR to cover the high-level operations - let me know if any obvious ones are missing. Instrumenting the lower level ones would result in much more (noisy) output.

Live link here: https://logfire-eu.pydantic.dev/public-trace/73de9eac-7379-4581-b285-845a7a52c56b?spanId=eddfab36dba4de22

Screenshot 2025-10-31 at 18 07 23

Re. documentation, let me know if you have a particular preference here e.g. if you want to stick with a news item (knitr), or have a separate vignette (mirai).

@krlmlr krlmlr changed the title OpenTelemetry Integration feat: Added support for [OpenTelemetry](https://opentelemetry.io/) observability. See ?otelsdk::collecting for more details on configuring OpenTelemetry Oct 31, 2025
Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. My understanding is that this is opt-in, and that tracing for DBI can be disabled even if tracing for other sources is enabled. I wonder if we can emit a banner message when connecting that points to relevant documentation?

@shikokuchuo
Copy link
Author

My understanding is that this is opt-in, and that tracing for DBI can be disabled even if tracing for other sources is enabled.

Yes, you're right - and detailed in the otelsdk instrumentation docs.

I wonder if we can emit a banner message when connecting that points to relevant documentation?

I'm thinking that in some cases it may be a system admin which has set up otel collection rather than the end user. So it may be surprising for the user to see a banner, especially as they wouldn't then know what to do with the information.

@hadley hadley changed the title feat: Added support for [OpenTelemetry](https://opentelemetry.io/) observability. See ?otelsdk::collecting for more details on configuring OpenTelemetry feat: Add support for OpenTelemetry Nov 3, 2025
@hadley
Copy link
Member

hadley commented Nov 3, 2025

As this rolls out across more packages, we'll do more to promote it, so hopefully folks start to internalise that this sort of observability is available in all the packages they rely on the most.

@shikokuchuo
Copy link
Author

I've now updated this PR with a common approach on caching the tracer, and a testing helper (following discussions with @schloerke who's been spearheading the otel integration in Shiny/promises).

@shikokuchuo shikokuchuo requested review from hadley and krlmlr November 10, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants