Skip to content
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

[MLOB-1981] fix(llmobs): childOf applies to LLM Observability spans #4997

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sabrenner
Copy link
Collaborator

@sabrenner sabrenner commented Dec 11, 2024

What does this PR do?

Has the LLMObs SDK respect the childOf option when instrumenting spans with LLM Observability. If childOf is specified, we will activate the function in the context of that span if it is an LLM Observability span, otherwise activate it in the scope of the current active LLMObs span.

This is useful for cases where you might want to have a callback run in the same scope as the function that invoked it:

let llmSpan

function myLlm () {
  setTimeout(myTask, 200)
  llmobs.trace({ ... }, span => { 
    llmSpan = span 
    // some other task that keeps this span alive for more than 200ms
  })
}

function myTask () {
  llmobs.trace({ ..., childOf: llmSpan }, () => ...)
}

as we do not have a way of currently activating myTask in the scope of the traced function, since the function is not called before the scope of the llmobs.trace in myLlm closes. Respecting the childOf argument helps with this.

Motivation

Fixes parentage in cases of manual instrumentation.

Copy link

Overall package size

Self size: 8.24 MB
Deduped: 94.81 MB
No deduping: 95.37 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.2.2 | 29.27 MB | 29.27 MB | | @datadog/native-appsec | 8.3.0 | 19.37 MB | 19.38 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.6.0 | 2.58 MB | 2.72 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.0.1 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@sabrenner sabrenner marked this pull request as ready for review December 11, 2024 17:37
@sabrenner sabrenner requested a review from a team as a code owner December 11, 2024 17:37
Copy link
Contributor

@Yun-Kim Yun-Kim left a comment

Choose a reason for hiding this comment

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

small question but otherwise lgtm. thanks!

@@ -355,6 +367,10 @@ class LLMObs extends NoopLLMObs {
}
}

_getParent (parent, childOf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if childOf is null (example: the queued task does not end up executing), do we handle this case here?

@sabrenner sabrenner marked this pull request as draft December 16, 2024 22:55
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

one small thought about the handling of unexpected values, otherwise good to me!

@@ -355,6 +367,10 @@ class LLMObs extends NoopLLMObs {
}
}

_getParent (parent, childOf) {
return childOf instanceof Span && LLMObsTagger.tagMap.has(childOf) ? childOf : parent
Copy link
Member

Choose a reason for hiding this comment

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

I think this condition would subtly hide programming errors with the type of childOf. I'd rather we just take the value of childOf if specified and let it fail wherever it might if it's the wrong type or fail hard up front in these cases rather than subtly falling back to parent here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants