-
Notifications
You must be signed in to change notification settings - Fork 13
Implements OpenTelemetry Instrumentation for Observability #395
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
base: main
Are you sure you want to change the base?
Conversation
3ba64ef
to
f90d20a
Compare
41027d9
to
13bfe39
Compare
Co-authored-by: Barret Schloerke <[email protected]> Signed-off-by: Charlie Gao <[email protected]>
if (otel_tracing) { | ||
spn <- otel::start_local_active_span( | ||
"mirai::mirai_map", | ||
links = list(compute_profile = ..[[.compute]][["otel_span"]]) |
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.
Will this work if .compute
is null?
Subsetting a list with a NULL key makes an error. (I don't know what ..
is in practice)
❯❯ list(a = 4)[[NULL]]
Error in list(a = 4)[[NULL]] :
attempt to select less than one element in get1index
❯❯ list2env(list(a = 4))[[NULL]]
Error in list2env(list(a = 4))[[NULL]] :
wrong arguments for subsetting an environment
if (otel_tracing) { | ||
dmn_spn <- otel::start_local_active_span( | ||
"mirai::daemon", | ||
attributes = otel::as_attributes(list(url = url)) | ||
) | ||
} |
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.
From conversations with @atheriel, should we switch the mirai long running daemons to be otel session start/end events?
Then, instead of links, the span would have a session.id
attribute pointing to the daemon session.
https://opentelemetry.io/docs/specs/semconv/general/session/
(untested)
spn <- otel::start_local_active_span(
"mirai::daemon->eval",
# links = list(daemon = dmn_spn),
options = list(kind = "server", parent = prtctx, session.id = dmn_spn$span_id)
)
Similarly, A session.start
and session.end
submit events to the active span and spn$add_event()
.
Maybe this? (untested)
spn <- otel::get_active_span()
spn$add_event("session.start", attributes = list(session.id = url))
And on daemon shutdown:
spn <- otel::get_active_span()
spn$add_event("session.end", attributes = list(session.id = url))
@@ -38,6 +38,7 @@ | |||
# tested implicitly | |||
|
|||
.onLoad <- function(libname, pkgname) { | |||
otel_tracing <<- requireNamespace("otel", quietly = TRUE) && otel::is_tracing_enabled() |
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.
My gut reaction is that this value should be dynamic and not fixed.
Motivation: otelsdk::with_otel_record(expr)
dynamically turns on tracing, which is useful for testing.
I bet you could reduce it down to a single call. If otel isn't tracing, then store NULL
for your span object just flag off of that span value. Then it is only called during the initial creation and not k
times throughout the mirai's lifecycle
Closes #394.
Current behaviour:
daemons(n)
creates a persistentmirai::daemons
spandaemon()
instance also creates a persistentmirai::daemon
spanmirai()
fires a shortmirai::mirai
client span, which inherits from the active otel span (and links to themirai::daemons
span)mirai::daemon->eval
for the duration of the evaluation (and links to the relevantmirai::daemon
span so we know where it ran)daemons(0)
ends the persistent spansTest using: