Skip to content

feat: enable open telemetry to capture the current module #1687

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.

Sign up for GitHub

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

Draft
osenan wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −0
Draft

feat: enable open telemetry to capture the current module #1687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,14 @@ Suggests:
knitr (>= 1.42),
mirai (>= 1.1.1),
MultiAssayExperiment,
otel(>= 0.2.0),
otelsdk(>= 0.2.2),

@averissimo averissimo Feb 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this?

@llrs-roche llrs-roche Feb 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

otel yes as it is used

@averissimo averissimo Feb 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

otelsdk in particular 😅 it's not being used in the PR

@osenan osenan Feb 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is not used by teal code by the dependency is necessary for Open Telemetry to work.

@llrs-roche llrs-roche Feb 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

otelsdk is a dependency of otel and it is only needed to collect telemtry so I am not sure why it is needed here

R6,
renv (>= 1.0.11),
rmarkdown (>= 2.23),
roxy.shinylive,
rvest (>= 1.0.0),
shiny (>= 1.12.1),
Comment thread
averissimo marked this conversation as resolved.
shinytest2,
shinyvalidate,
testthat (>= 3.2.0),
Expand Down
4 changes: 4 additions & 0 deletions R/module_nested_tabs.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ srv_teal_module <- function(id,
reporter = teal.reporter::Reporter$new(),
data_load_status = reactive("ok")) {
moduleServer(id, function(input, output, session) {
observeEvent(input$active_module_id, {

@averissimo averissimo Feb 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1. Where does teal.logger stand, would adding otel support to logger be a better choice (controlled by teal options).

We already have a logging infrastructure, and adding this parallel method seems redundant.

In particular with the question

What is the criteria for the framework/app/module developer to use otel vs. teal.logger?

This approach could save all INFO level to otel (when otel is enabled).

The verbosity of INFO+ levels isn't that bad as only messages() calls generate at this level IIRC (other than in t.m.helios)

2. Would a new API like wrap_server() be a better fit for this use case?

My main question is if we should support this type of logging natively on teal and static? I can imagine that there are different use cases to extend past by the module that's being used.

Let's say for an application that is deployed on connect, we may want to also log the username to assess if the modules usage are overrepresented by power users.

I would like to resurface the idea of a API that extends the init() call to allow for custom server/ui logic for the whole of teal without the need of using it as module.

my_data <- teal.data::teal_data(IRIS = iris)
teal::init(data = my_data, modules  = teal::example_module()) |>
  teal::transform_init(
    srv = function(id) {
      shiny::moduleServer(id, function(input, output, session) {
          if (otel::is_tracing_enabled()) {
           ospan <- otel::get_active_span()
           ospan$set_attribute("user_role", "admin")
           ospan$set_attribute("app_version", "1.2.3")
           ospan$set_attribute("app_name", "best_teal_app")
         }
        shiny::observeEvent(session$input[["teal-teal_modules-active_module_id"]], { # namespace depends on where we run this modifier
        otel::log_info(sprintf("The app current module is %s", input$active_module_id))
      })
    },
    ui = NULL # nothing to add here
  )

@llrs-roche llrs-roche Feb 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. I agree that we should reuse the infrastructure we already have. Reporting all info to otel would be good.
  2. I like the idea of being able to know what modules each user uses. Isn't this proposed API similar to the after() function but applied to the whole app?

@averissimo averissimo Feb 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

3. How can we trace which application is being logged?

How can we trace the actual application name? Skimming from the documentation I see that there's an API for attributes.

How would that work with this implementation?

Image

@gogonzo gogonzo Feb 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

active_module_id won't give us a clear picture about what module is used as this is a derivative of a module$label. This could be anything. For Example I can use tm_g_km and call it "My awesome module" - input will be my_awesome_module (or something like this).
If we want to know how often a module is used I would suggest to implement this in teal.modules.* (where teal.modules.* logs are implemented wrongly :D).

We can't accept the fact of having two log-frameworks (logger and otel)

@donyunardi donyunardi Feb 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree, it should capture the module's label, the module's function name, and adding more to the scope, the name of the teal app.

otel::log_info(sprintf("The app current module is %s", input$active_module_id))

@llrs-roche llrs-roche Feb 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

otel is in suggested, code using it should be conditional of otel being loaded:

Suggested change
otel::log_info(sprintf("The app current module is %s", input$active_module_id))
if (requireNamespace("otel", quiet = TRUE)) otel::log_info(sprintf("The app current module is %s", input$active_module_id))

We might want to report the id and the label as the label might be more human readable while the id is more for machines.

})

.srv_teal_module(
id = "nav",
data = data,
Expand Down
Loading