From 05b0f270c47aff208f9eded5a84c56bafc9b6a9e Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 8 Dec 2025 14:55:59 -0500 Subject: [PATCH] fix(otel): ExtendedTask's otel enabled status set during init (#4334) --- NEWS.md | 1 + R/extended-task.R | 29 ++- man/ExtendedTask.Rd | 23 +++ tests/testthat/helper-otel.R | 31 ++++ tests/testthat/test-otel-extended-task.R | 219 +++++++++++++++++++++++ tests/testthat/test-otel-label.R | 21 +-- tests/testthat/test-otel-shiny.R | 32 ---- 7 files changed, 310 insertions(+), 46 deletions(-) create mode 100644 tests/testthat/helper-otel.R create mode 100644 tests/testthat/test-otel-extended-task.R diff --git a/NEWS.md b/NEWS.md index 5134e94f2..8783f4b78 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # shiny (development version) +* `ExtendedTask` now captures the OpenTelemetry recording state at initialization time rather than at invocation time, ensuring consistent span recording behavior regardless of runtime configuration changes. (#4334) * Added `withOtelCollect()` and `localOtelCollect()` functions to temporarily control OpenTelemetry collection levels during reactive expression creation. These functions allow you to enable or disable telemetry collection for specific modules or sections of code where reactive expressions are being created. (#4333) * OpenTelemetry code attributes now include both preferred (`code.file.path`, `code.line.number`, `code.column.number`) and deprecated (`code.filepath`, `code.lineno`, `code.column`) attribute names to follow OpenTelemetry semantic conventions while maintaining backward compatibility. The deprecated names will be removed in a future release after Logfire supports the preferred names. (#4325) * Timer tests are now skipped on CRAN. (#4327) diff --git a/R/extended-task.R b/R/extended-task.R index 0c3f001e5..fc965b51b 100644 --- a/R/extended-task.R +++ b/R/extended-task.R @@ -41,6 +41,28 @@ #' is, a function that quickly returns a promise) and allows even that very #' session to immediately unblock and carry on with other user interactions. #' +#' @section OpenTelemetry Integration: +#' +#' When an `ExtendedTask` is created, if OpenTelemetry tracing is enabled for +#' `"reactivity"` (see [withOtelCollect()]), the `ExtendedTask` will record +#' spans for each invocation of the task. The tracing level at `invoke()` time +#' does not affect whether spans are recorded; only the tracing level when +#' calling `ExtendedTask$new()` matters. +#' +#' The OTel span will be named based on the label created from the variable the +#' `ExtendedTask` is assigned to. If no label can be determined, the span will +#' be named ``. Similar to other Shiny OpenTelemetry spans, the span +#' will also include source reference attributes and session ID attributes. +#' +#' ```r +#' withOtelCollect("all", { +#' my_task <- ExtendedTask$new(function(...) { ... }) +#' }) +#' +#' # Span recorded for this invocation: ExtendedTask my_task +#' my_task$invoke(...) +#' ``` +#' #' @examplesIf rlang::is_interactive() && rlang::is_installed("mirai") #' library(shiny) #' library(bslib) @@ -143,6 +165,10 @@ ExtendedTask <- R6Class("ExtendedTask", portable = TRUE, cloneable = FALSE, otel_srcref_attributes(call_srcref, "ExtendedTask"), otel_session_id_attrs(domain) ) %||% list() + + # Capture this value at init-time, not run-time + # This way, the span is only created if otel was enabled at time of creation... just like other spans + private$is_recording_otel <- has_otel_collect("reactivity") }, #' @description #' Starts executing the long-running operation. If this `ExtendedTask` is @@ -175,7 +201,7 @@ ExtendedTask <- R6Class("ExtendedTask", portable = TRUE, cloneable = FALSE, private$invocation_queue$add(list(args = args, call = call)) } else { - if (has_otel_collect("reactivity")) { + if (private$is_recording_otel) { private$otel_span <- start_otel_span( private$otel_span_label, attributes = private$otel_attrs @@ -253,6 +279,7 @@ ExtendedTask <- R6Class("ExtendedTask", portable = TRUE, cloneable = FALSE, otel_span_label = NULL, otel_log_label_add_to_queue = NULL, otel_attrs = list(), + is_recording_otel = FALSE, otel_span = NULL, do_invoke = function(args, call = NULL) { diff --git a/man/ExtendedTask.Rd b/man/ExtendedTask.Rd index 6e46ddc0c..9a2b1d6f9 100644 --- a/man/ExtendedTask.Rd +++ b/man/ExtendedTask.Rd @@ -45,6 +45,29 @@ is, a function that quickly returns a promise) and allows even that very session to immediately unblock and carry on with other user interactions. } +\section{OpenTelemetry Integration}{ + + +When an \code{ExtendedTask} is created, if OpenTelemetry tracing is enabled for +\code{"reactivity"} (see \code{\link[=withOtelCollect]{withOtelCollect()}}), the \code{ExtendedTask} will record +spans for each invocation of the task. The tracing level at \code{invoke()} time +does not affect whether spans are recorded; only the tracing level when +calling \code{ExtendedTask$new()} matters. + +The OTel span will be named based on the label created from the variable the +\code{ExtendedTask} is assigned to. If no label can be determined, the span will +be named \verb{}. Similar to other Shiny OpenTelemetry spans, the span +will also include source reference attributes and session ID attributes. + +\if{html}{\out{
}}\preformatted{withOtelCollect("all", \{ + my_task <- ExtendedTask$new(function(...) \{ ... \}) +\}) + +# Span recorded for this invocation: ExtendedTask my_task +my_task$invoke(...) +}\if{html}{\out{
}} +} + \examples{ \dontshow{if (rlang::is_interactive() && rlang::is_installed("mirai")) withAutoprint(\{ # examplesIf} library(shiny) diff --git a/tests/testthat/helper-otel.R b/tests/testthat/helper-otel.R new file mode 100644 index 000000000..f32e81863 --- /dev/null +++ b/tests/testthat/helper-otel.R @@ -0,0 +1,31 @@ +# Helper function to create a mock otel span +create_mock_otel_span <- function(name = "test_span") { + structure( + list( + name = name, + activate = function(...) NULL, + end = function(...) NULL + ), + class = "otel_span" + ) +} + +# Helper function to create a mock tracer +create_mock_tracer <- function() { + structure( + list( + name = "mock_tracer", + is_enabled = function() TRUE, + start_span = function(name, ...) create_mock_otel_span(name) + ), + class = "otel_tracer" + ) +} + +# Helper function to create a mock logger +create_mock_logger <- function() { + structure( + list(name = "mock_logger"), + class = "otel_logger" + ) +} diff --git a/tests/testthat/test-otel-extended-task.R b/tests/testthat/test-otel-extended-task.R new file mode 100644 index 000000000..125659390 --- /dev/null +++ b/tests/testthat/test-otel-extended-task.R @@ -0,0 +1,219 @@ +# Tests for ExtendedTask otel behavior + +ex_task_42 <- function() { + ExtendedTask$new(function() { + promises::promise_resolve(42) + }) +} + +test_that("ExtendedTask captures otel collection state at initialization", { + # Test that has_otel_collect is called at init, not at invoke time + withr::local_options(list(shiny.otel.collect = "reactivity")) + + # Enable otel tracing + local_mocked_bindings( + otel_is_tracing_enabled = function() TRUE + ) + + task <- ex_task_42() + + # Check that is_recording_otel is captured at init time + expect_true(task$.__enclos_env__$private$is_recording_otel) +}) + +test_that("ExtendedTask sets is_recording_otel to FALSE when otel disabled", { + # Enable otel tracing + local_mocked_bindings( + otel_is_tracing_enabled = function() FALSE + ) + + # Test with all level + withr::with_options(list(shiny.otel.collect = "all"), { + task1 <- ex_task_42() + expect_false(task1$.__enclos_env__$private$is_recording_otel) + }) + + # Test with reactivity level + withr::with_options(list(shiny.otel.collect = "reactivity"), { + task1 <- ex_task_42() + expect_false(task1$.__enclos_env__$private$is_recording_otel) + }) + + # Test with session level (should be FALSE) + withr::with_options(list(shiny.otel.collect = "session"), { + task2 <- ex_task_42() + expect_false(task2$.__enclos_env__$private$is_recording_otel) + }) + + # Test with none level (should be FALSE) + withr::with_options(list(shiny.otel.collect = "none"), { + task3 <- ex_task_42() + expect_false(task3$.__enclos_env__$private$is_recording_otel) + }) +}) + +test_that("ExtendedTask sets is_recording_otel based on has_otel_collect at init", { + # Enable otel tracing + local_mocked_bindings( + otel_is_tracing_enabled = function() TRUE + ) + + # Test with all level + withr::with_options(list(shiny.otel.collect = "all"), { + task1 <- ex_task_42() + expect_true(task1$.__enclos_env__$private$is_recording_otel) + }) + + # Test with reactivity level + withr::with_options(list(shiny.otel.collect = "reactivity"), { + task1 <- ex_task_42() + expect_true(task1$.__enclos_env__$private$is_recording_otel) + }) + + # Test with session level (should be FALSE) + withr::with_options(list(shiny.otel.collect = "session"), { + task2 <- ex_task_42() + expect_false(task2$.__enclos_env__$private$is_recording_otel) + }) + + # Test with none level (should be FALSE) + withr::with_options(list(shiny.otel.collect = "none"), { + task3 <- ex_task_42() + expect_false(task3$.__enclos_env__$private$is_recording_otel) + }) +}) + +test_that("ExtendedTask uses init-time otel setting even if option changes later", { + + # Enable otel tracing + local_mocked_bindings( + otel_is_tracing_enabled = function() TRUE + ) + + # Test that changing the option after init doesn't affect the task + withr::with_options(list(shiny.otel.collect = "reactivity"), { + task <- ex_task_42() + }) + + # Capture the initial state + expect_true(task$.__enclos_env__$private$is_recording_otel) + + # Change the option after initialization + withr::with_options(list(shiny.otel.collect = "none"), { + # The task should still have the init-time setting + expect_true(task$.__enclos_env__$private$is_recording_otel) + }) + +}) + +test_that("ExtendedTask respects session level otel collection", { + # Test that session level doesn't enable reactivity spans + withr::local_options(list(shiny.otel.collect = "session")) + + task <- ex_task_42() + + # Should not record otel at session level + expect_false(task$.__enclos_env__$private$is_recording_otel) +}) + +test_that("ExtendedTask respects reactive_update level otel collection", { + # Test that reactive_update level doesn't enable reactivity spans + withr::local_options(list(shiny.otel.collect = "reactive_update")) + + task <- ex_task_42() + + # Should not record otel at reactive_update level + expect_false(task$.__enclos_env__$private$is_recording_otel) +}) + +test_that("ExtendedTask creates span only when is_recording_otel is TRUE", { + # Test that span is only created when otel is enabled + withr::local_options(list(shiny.otel.collect = "reactivity")) + + span_created <- FALSE + + local_mocked_bindings( + start_otel_span = function(...) { + span_created <<- TRUE + create_mock_otel_span("extended_task") + }, + otel_is_tracing_enabled = function() TRUE + ) + + with_shiny_otel_record({ + withReactiveDomain(MockShinySession$new(), { + task <- ex_task_42() + + # Reset the flag + span_created <- FALSE + + # Invoke the task + isolate({ + task$invoke() + }) + }) + }) + + + # Span should have been created because is_recording_otel is TRUE + expect_true(span_created) +}) + +test_that("ExtendedTask does not create span when is_recording_otel is FALSE", { + # Test that span is not created when otel is disabled + withr::local_options(list(shiny.otel.collect = "none")) + + span_created <- FALSE + + local_mocked_bindings( + start_otel_span = function(...) { + span_created <<- TRUE + create_mock_otel_span("extended_task") + } + ) + + withReactiveDomain(MockShinySession$new(), { + task <- ex_task_42() + + # Invoke the task + isolate({ + task$invoke() + }) + }) + + # Span should not have been created because is_recording_otel is FALSE + expect_false(span_created) +}) + + +test_that("Multiple ExtendedTask invocations use same is_recording_otel value", { + # Enable otel tracing + withr::local_options(list(shiny.otel.collect = "reactivity")) + local_mocked_bindings( + otel_is_tracing_enabled = function() TRUE + ) + + withReactiveDomain(MockShinySession$new(), { + task <- ex_task_42() + + # Verify is_recording_otel is TRUE at init + expect_true(task$.__enclos_env__$private$is_recording_otel) + + # Change option after initialization (should not affect the task) + withr::with_options( + list(shiny.otel.collect = "none"), + { + # The task should still have the init-time setting + expect_true(task$.__enclos_env__$private$is_recording_otel) + + # Verify is_recording_otel doesn't change on invocation + isolate({ + task$invoke() + }) + + # Still should be TRUE after invoke + expect_true(task$.__enclos_env__$private$is_recording_otel) + } + ) + }) +}) diff --git a/tests/testthat/test-otel-label.R b/tests/testthat/test-otel-label.R index 59bbe58d9..b47aeeb94 100644 --- a/tests/testthat/test-otel-label.R +++ b/tests/testthat/test-otel-label.R @@ -68,9 +68,12 @@ test_that("reactive bindCache labels are created", { }) test_that("ExtendedTask otel labels are created", { - ex_task <- ExtendedTask$new(function() { promises::then(promises::promise_resolve(42), force) }) + # Record everything + localOtelCollect("all") info <- with_shiny_otel_record({ + ex_task <- ExtendedTask$new(function() { promises::then(promises::promise_resolve(42), force) }) + ex_task$invoke() while(!later::loop_empty()) { later::run_now() @@ -79,30 +82,22 @@ test_that("ExtendedTask otel labels are created", { trace <- info$traces[[1]] - expect_equal( - trace$name, - "ExtendedTask ex_task" - ) - + expect_equal(trace$name, "ExtendedTask ex_task") + # Module test withReactiveDomain(MockShinySession$new(), { - ex2_task <- ExtendedTask$new(function() { promises::then(promises::promise_resolve(42), force) }) - info <- with_shiny_otel_record({ + ex2_task <- ExtendedTask$new(function() { promises::then(promises::promise_resolve(42), force) }) ex2_task$invoke() while(!later::loop_empty()) { later::run_now() } }) - }) trace <- info$traces[[1]] - expect_equal( - trace$name, - "ExtendedTask mock-session:ex2_task" - ) + expect_equal(trace$name, "ExtendedTask mock-session:ex2_task") }) diff --git a/tests/testthat/test-otel-shiny.R b/tests/testthat/test-otel-shiny.R index d5eaa0fe3..48025195c 100644 --- a/tests/testthat/test-otel-shiny.R +++ b/tests/testthat/test-otel-shiny.R @@ -1,37 +1,5 @@ # Tests for otel-shiny.R functions -# Helper function to create a mock otel span -create_mock_otel_span <- function(name = "test_span") { - structure( - list( - name = name, - activate = function(...) NULL, - end = function(...) NULL - ), - class = "otel_span" - ) -} - -# Helper function to create a mock tracer -create_mock_tracer <- function() { - structure( - list( - name = "mock_tracer", - is_enabled = function() TRUE, - start_span = function(name, ...) create_mock_otel_span(name) - ), - class = "otel_tracer" - ) -} - -# Helper function to create a mock logger -create_mock_logger <- function() { - structure( - list(name = "mock_logger"), - class = "otel_logger" - ) -} - test_that("otel_tracer_name constant is correct", { expect_equal(otel_tracer_name, "co.posit.r-package.shiny") })