-
Notifications
You must be signed in to change notification settings - Fork 309
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-1804] feat(langchain): add langchain instrumentation #4860
Conversation
Overall package sizeSelf size: 8.09 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.5.0 | 2.51 MB | 2.65 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 | | 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 | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4860 +/- ##
===========================================
+ Coverage 79.17% 91.16% +11.99%
===========================================
Files 273 129 -144
Lines 12427 4461 -7966
===========================================
- Hits 9839 4067 -5772
+ Misses 2588 394 -2194 ☔ View full report in Codecov by Sentry. |
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.
Great work so far! Left a couple questions but mostly looks good 👍
|
||
// OpenAI (and Embeddings in general) do not define an lc_namespace | ||
const namespace = ['langchain', 'embeddings', 'openai'] | ||
shimmer.wrap(OpenAIEmbeddings.prototype, 'embedDocuments', embedDocuments => |
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.
In langchain's python implementation they made embedDocuments/embedQuery abstract methods which made it gross when patching the embedDocuments/Query methods (i.e. patching the base class method does not wrap the individual embedding classes). Is that not the case for langchain's node JS library?
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.
It's actually the same case for Node.js library, as these abstract methods in TypeScript are not compiled into properties we can patch in plain JS. So I'm only doing the OpenAI embedding patching here, as I tried to patch the base Embeddings
class and couldn't 😞
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.
ref, which is an 'outdated' comment but really isn't, i think it still holds for this point 😅
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.
Understood, we should actually chat more about supporting edge cases like this (let's bring this up in our core-obs sync next week)
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.
typically in patterns like this there is a common place where the objects are instantiated or interacted with early on that we can use as a hook point for patching/wrapping. But yeah it's annoying that it's not generalizable.
|
||
const tags = {} | ||
|
||
// TODO: do we need token tagging |
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.
We should have token metrics get tagged, I know Langchain should support token metrics for openai at least
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.
to clarify this (as my TODO isn't super helpful beyond face value 😅), referring to this in our LangChain integration, I don't believe we have an equivalent get_openai_token_cost_for_model
available to us through LangChain. Should we just do the langchain.tokens.{prompt, completion, total}_tokens
tag on the span here instead and forgo the cost metric?
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.
Does langchain expose token metrics on the returned OpenAI response object? If so, should be as simple as extracting those token metrics and setting them on the span tags. Let me know otherwise 👍
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.
yep we can grab the token metrics themselves. i'll add them as tags!
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.
added this in for openai
response objects: 0ecb096. when I do the llmobs
integration, i'll try to make it provider-agnostic as we did for the Python LLMObs integration.
tags[`langchain.response.outputs.${idx}.embedding_length`] = output.length | ||
} | ||
} else { | ||
tags['langchain.response.outputs.embedding_length'] = result.length | ||
} |
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.
This is fine as is, but I wonder if we should just tag one embedding length tag instead of one per entry since every model will output the same dimension embeddings for every input (since dimensions are a model-specific param AFAIK).
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.
ah yes good point. I think I lifted this right from the Python integration, so as long we're ok deviating here, I think it probably does make more sense to just have one tag
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.
Haha yup I noticed we do the same in the Python integration, but wanted to make sure we don't repeat the same mistakes when possible (it'll be harder to remove this from the Python integration but I'd rather we not have this unnecessary tagging to begin with)
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.
yep makes sense. i added it in this commit
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.
did a first pass. Really like the handler design for splitting out the span logic between components.
Just a few smaller things but don't see anything majorly wrong.
Nice work 👏 👏
|
||
// OpenAI (and Embeddings in general) do not define an lc_namespace | ||
const namespace = ['langchain', 'embeddings', 'openai'] | ||
shimmer.wrap(OpenAIEmbeddings.prototype, 'embedDocuments', embedDocuments => |
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.
typically in patterns like this there is a common place where the objects are instantiated or interacted with early on that we can use as a hook point for patching/wrapping. But yeah it's annoying that it's not generalizable.
for (const [key, value] of Object.entries(input)) { | ||
// these are mappings to the python client names, ie lc_kwargs | ||
// only present on BaseMessage types | ||
if (key.includes('lc')) continue |
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.
Maybe I'm missing something but I'm not following why we're skipping lc
-containing keys here. Are they internal to LangChain?
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.
they are internal fields, and often don't hold relevant information (there's a field called lc_serializable
, which is just a boolean not relevant to the chain), or have duplicate information (ie lc_namespace
, which we use for the resource name, or lc_kwargs
, which are duplicates of some of the input values, which we already tag).
while some of them actually won't pass the truncate
/normalize check, since they are not strings, i added this to be safe/not add useless tags. can add them on down the road if there's a use case for them!
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.
ah ok I see! should the string check be startsWith('lc_')
then?
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.
i think that's fair!
|
||
for (const messageIndex in messageSet) { | ||
const message = messageSet[messageIndex] | ||
if (this.isPromptCompletionSampled()) { |
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.
is the sampling call cached or really cheap? Might be worth to memoize here
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.
good question - the sampler just uses Math.random
to compute sampled. the call itself should be fairly cheap
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.
nothing blocking, good stuff!
const max = this.config.spanCharLimit | ||
|
||
text = text | ||
.replace(RE_NEWLINE, '\\n') |
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.
Do you ever have to deal with Windows newlines? e.g. \r\n
?
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.
Probably not if this is a server response. But if it's data provided from the app or user then it could be a concern.
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.
it could be data from the user, as we'll use this function to truncate input text (and then also output text from the server response)
34c86a4
if (!text) return | ||
if (typeof text !== 'string' || !text || (typeof text === 'string' && text.length === 0)) return | ||
|
||
const max = this.config.spanCharLimit |
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.
[q] How is this value initialized? It seems like we have a few different *SpanCharLimit
variables, but I'm not clear on how this config got set
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.
yeah good question. we initialize these handlers here, which pass in the tracer config's langchain
property, which we set defaults for and read from env to populate.
I'll be refactoring this integration along with OpenAI to have shared logic for this truncation and prompt/completion sampling, so when I do I'll probably rename this variable to be langchainConfig
or something 😅
* wip * wip * first pass at chain invoke and chat,llm generate * add langchain openai embeddings * add batch call * change api key logic * testing * ts def changes * codeowners changes * add clarifying issue as reason for skipping esm tests * fix langchain patching for possible esm files vs commonjs files, namespace * configurable truncation and prompt completion sampling * remove unneeded util file * remove some unneeded code * fix patching esm vs cjs issues * json stringify non-string chain outputs * apikey, model, provider should no-op by default * add some token handling logic * review comments * check lc_ for ignored properties
* wip * wip * first pass at chain invoke and chat,llm generate * add langchain openai embeddings * add batch call * change api key logic * testing * ts def changes * codeowners changes * add clarifying issue as reason for skipping esm tests * fix langchain patching for possible esm files vs commonjs files, namespace * configurable truncation and prompt completion sampling * remove unneeded util file * remove some unneeded code * fix patching esm vs cjs issues * json stringify non-string chain outputs * apikey, model, provider should no-op by default * add some token handling logic * review comments * check lc_ for ignored properties
* wip * wip * first pass at chain invoke and chat,llm generate * add langchain openai embeddings * add batch call * change api key logic * testing * ts def changes * codeowners changes * add clarifying issue as reason for skipping esm tests * fix langchain patching for possible esm files vs commonjs files, namespace * configurable truncation and prompt completion sampling * remove unneeded util file * remove some unneeded code * fix patching esm vs cjs issues * json stringify non-string chain outputs * apikey, model, provider should no-op by default * add some token handling logic * review comments * check lc_ for ignored properties
* wip * wip * first pass at chain invoke and chat,llm generate * add langchain openai embeddings * add batch call * change api key logic * testing * ts def changes * codeowners changes * add clarifying issue as reason for skipping esm tests * fix langchain patching for possible esm files vs commonjs files, namespace * configurable truncation and prompt completion sampling * remove unneeded util file * remove some unneeded code * fix patching esm vs cjs issues * json stringify non-string chain outputs * apikey, model, provider should no-op by default * add some token handling logic * review comments * check lc_ for ignored properties
This is great, very excited for this. Do you have any insight into ESM support timelines or workarounds? Thank you! |
What does this PR do?
Adds initial instrumentation support for LangChain
>=0.1
, specifically for:chain.invoke
andchain.batch
(from@langchain/core/runnables/base
)chat_model.generate
(and inherentlychat_model.invoke
, from@langchain/core/language_models/chat_models
)llm.generate
(and inherentlyllm.invoke
, from@langchain/core/language_models/llms
)openaiEmbeddings.embedQuery
(from@langchain/openai/embeddings
)openaiEmbeddings.embedDocuments
(from@langchain/openai/embeddings
)We're restricting to
>=0.1
as versions before are slated for deprecation in an upcoming release, although it can be added by request. Additionally,>=0.1
has stable support for LCEL invocations, which this PR adds support for.This instrumentation will happen on any
langchain
,@langchain/core
, or@langchain/openai
imports.Additionally, we're adding
DD_LANGCHAIN_SPAN_CHAR_LIMIT
andDD_LANGCHAIN_SPAN_PROMPT_COMPLETION_SAMPLE_RATE
to control the length of truncated text (I/O) and rate of sampling for prompts and completions, respectively.Motivation
Wanting to add an initial APM integration. MLOB will later build off our own LLMObs integration as well, and continue building this integration & support new features.
Plugin Checklist
Additional Notes
Will be putting up docs PRs separately.
Additionally, there is also an ESM issue with LangChain, specifically with LangSmith, which is not a necessary module for using LangChain but is bundled with LangChain nonetheless. I will be opening a separate issue for this.
I will additionally also be investigating if there is a workaround possible for this scenario. This PR will not include fixes for the ESM usage of
langchain
, but does include patching (by patching both thejs
andcjs
extensions)