From 0d8aea0748e67fbcca881fda40f530e9fbed7ef7 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Fri, 27 Sep 2024 15:54:55 -0400 Subject: [PATCH 01/17] span processor --- .../dd-trace/src/exporters/agent/index.js | 6 + packages/dd-trace/src/format.js | 4 +- .../dd-trace/src/llmobs/span_processor.js | 147 ++++++++++++++++++ packages/dd-trace/src/llmobs/util.js | 9 +- packages/dd-trace/src/span_processor.js | 3 + 5 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 packages/dd-trace/src/llmobs/span_processor.js diff --git a/packages/dd-trace/src/exporters/agent/index.js b/packages/dd-trace/src/exporters/agent/index.js index b2f25eeda99..58dd7b35b95 100644 --- a/packages/dd-trace/src/exporters/agent/index.js +++ b/packages/dd-trace/src/exporters/agent/index.js @@ -42,6 +42,12 @@ class AgentExporter { } export (spans) { + const { llmobs } = this._config + if (llmobs.enabled && llmobs.agentlessEnabled) { + log.debug('LLMObs agentless mode enabled. Not sending APM spans to the agent.') + return + } + this._writer.append(spans) const { flushInterval } = this._config diff --git a/packages/dd-trace/src/format.js b/packages/dd-trace/src/format.js index 1b7b86d17f0..230a04fbc9c 100644 --- a/packages/dd-trace/src/format.js +++ b/packages/dd-trace/src/format.js @@ -160,7 +160,9 @@ function extractTags (trace, span) { break } default: // eslint-disable-line no-fallthrough - addTag(trace.meta, trace.metrics, tag, tags[tag]) + if (!tag.startsWith('_ml_obs')) { // don't add ml_obs-related tags + addTag(trace.meta, trace.metrics, tag, tags[tag]) + } } } setSingleSpanIngestionTags(trace, context._spanSampling) diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js new file mode 100644 index 00000000000..4724f64334c --- /dev/null +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -0,0 +1,147 @@ +'use strict' + +const { + SPAN_KIND, + MODEL_NAME, + MODEL_PROVIDER, + METADATA, + INPUT_MESSAGES, + INPUT_VALUE, + OUTPUT_MESSAGES, + INPUT_DOCUMENTS, + OUTPUT_DOCUMENTS, + OUTPUT_VALUE, + METRICS, + ML_APP, + TAGS, + PARENT_ID_KEY, + SESSION_ID, + NAME +} = require('./constants') + +const { + ERROR_MESSAGE, + ERROR_TYPE, + ERROR_STACK +} = require('../constants') + +const AgentlessWriter = require('./writers/spans/agentless') +const AgentProxyWriter = require('./writers/spans/agentProxy') +const { isLLMSpan } = require('./util') + +const tracerVersion = require('../../../../package.json').version + +class LLMObsSpanProcessor { + constructor (config) { + this._config = config + const { llmobs } = config + + if (llmobs.enabled) { + const LLMObsSpanWriter = llmobs.agentlessEnabled ? AgentlessWriter : AgentProxyWriter + this._writer = new LLMObsSpanWriter(config) + } + } + + process (span) { + if (!this._config.llmobs.enabled) return + if (!isLLMSpan(span)) return + const payload = this._process(span) + + this._writer.append(payload) + } + + _process (span) { + const tags = span.context()._tags + const spanKind = tags[SPAN_KIND] + + const meta = { 'span.kind': spanKind, input: {}, output: {} } + const input = {} + const output = {} + + if (['llm', 'embedding'].includes(spanKind) && tags[MODEL_NAME]) { + meta.model_name = tags[MODEL_NAME] + meta.model_provider = (tags[MODEL_PROVIDER] || 'custom').toLowerCase() + } + if (tags[METADATA]) { + meta.metadata = JSON.parse(tags[METADATA]) + } + if (spanKind === 'llm' && tags[INPUT_MESSAGES]) { + input.messages = JSON.parse(tags[INPUT_MESSAGES]) + } + if (tags[INPUT_VALUE]) { + input.value = tags[INPUT_VALUE] + } + if (spanKind === 'llm' && tags[OUTPUT_MESSAGES]) { + output.messages = JSON.parse(tags[OUTPUT_MESSAGES]) + } + if (spanKind === 'embedding' && tags[INPUT_DOCUMENTS]) { + input.documents = JSON.parse(tags[INPUT_DOCUMENTS]) + } + if (tags[OUTPUT_VALUE]) { + output.value = tags[OUTPUT_VALUE] + } + if (spanKind === 'retrieval' && tags[OUTPUT_DOCUMENTS]) { + output.documents = JSON.parse(tags[OUTPUT_DOCUMENTS]) + } + + const error = tags.error + if (error) { + meta[ERROR_MESSAGE] = tags[ERROR_MESSAGE] || error.message || error.code + meta[ERROR_TYPE] = tags[ERROR_TYPE] || error.name + meta[ERROR_STACK] = tags[ERROR_STACK] || error.stack + } + + if (input) meta.input = input + if (output) meta.output = output + + const metrics = JSON.parse(tags[METRICS] || '{}') + + const mlApp = tags[ML_APP] + const sessionId = tags[SESSION_ID] + const parentId = tags[PARENT_ID_KEY] + + const name = tags[NAME] || span._name + + const llmObsSpanEvent = { + trace_id: span.context().toTraceId(true), + span_id: span.context().toSpanId(), + parent_id: parentId, + name, + tags: this._processTags(span, mlApp, sessionId, error), + start_ns: Math.round(span._startTime * 1e6), + duration: Math.round(span._duration * 1e6), + status: tags.error ? 'error' : 'ok', + meta, + metrics, + _dd: { + span_id: span.context().toSpanId(), + trace_id: span.context().toTraceId(true) + } + } + + if (sessionId) llmObsSpanEvent.session_id = sessionId + + return llmObsSpanEvent + } + + _processTags (span, mlApp, sessionId, error) { + let tags = { + version: this._config.version, + env: this._config.env, + service: this._config.service, + source: 'integration', + ml_app: mlApp, + 'dd-trace.version': tracerVersion, + error: Number(!!error) || 0, + language: 'javascript' + } + const errType = span.context()._tags[ERROR_TYPE] || error?.name + if (errType) tags.error_type = errType + if (sessionId) tags.session_id = sessionId + const existingTags = JSON.parse(span.context()._tags[TAGS] || '{}') + if (existingTags) tags = { ...tags, ...existingTags } + return Object.entries(tags).map(([key, value]) => `${key}:${value ?? ''}`) + } +} + +module.exports = LLMObsSpanProcessor diff --git a/packages/dd-trace/src/llmobs/util.js b/packages/dd-trace/src/llmobs/util.js index 5159a72b3b8..c6463346eeb 100644 --- a/packages/dd-trace/src/llmobs/util.js +++ b/packages/dd-trace/src/llmobs/util.js @@ -1,5 +1,7 @@ 'use strict' +const { SPAN_TYPE } = require('../../../../ext/tags') + function encodeUnicode (str) { if (!str) return str return str.split('').map(char => { @@ -11,6 +13,11 @@ function encodeUnicode (str) { }).join('') } +function isLLMSpan (span) { + return ['llm', 'openai'].includes(span?.context()._tags[SPAN_TYPE]) +} + module.exports = { - encodeUnicode + encodeUnicode, + isLLMSpan } diff --git a/packages/dd-trace/src/span_processor.js b/packages/dd-trace/src/span_processor.js index 6dc19407d56..870ede6fdfd 100644 --- a/packages/dd-trace/src/span_processor.js +++ b/packages/dd-trace/src/span_processor.js @@ -6,6 +6,7 @@ const SpanSampler = require('./span_sampler') const GitMetadataTagger = require('./git_metadata_tagger') const { SpanStatsProcessor } = require('./span_stats') +const LLMObsSpanProcessor = require('./llmobs/span_processor') const startedSpans = new WeakSet() const finishedSpans = new WeakSet() @@ -20,6 +21,7 @@ class SpanProcessor { this._stats = new SpanStatsProcessor(config) this._spanSampler = new SpanSampler(config.sampler) this._gitMetadataTagger = new GitMetadataTagger(config) + this._llmobs = new LLMObsSpanProcessor(config) } process (span) { @@ -42,6 +44,7 @@ class SpanProcessor { for (const span of started) { if (span._duration !== undefined) { + this._llmobs.process(span) const formattedSpan = format(span) this._stats.onSpanFinished(formattedSpan) formatted.push(formattedSpan) From c6e2dba992d16af69179372ea64df5b8e5bf4c6b Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Fri, 27 Sep 2024 15:55:04 -0400 Subject: [PATCH 02/17] tests --- .../test/exporters/agent/exporter.spec.js | 36 +- packages/dd-trace/test/format.spec.js | 10 + .../test/llmobs/span_processor.spec.js | 356 ++++++++++++++++++ packages/dd-trace/test/llmobs/util.spec.js | 27 +- packages/dd-trace/test/span_processor.spec.js | 39 +- 5 files changed, 457 insertions(+), 11 deletions(-) create mode 100644 packages/dd-trace/test/llmobs/span_processor.spec.js diff --git a/packages/dd-trace/test/exporters/agent/exporter.spec.js b/packages/dd-trace/test/exporters/agent/exporter.spec.js index a3e402ba358..67ba988ce67 100644 --- a/packages/dd-trace/test/exporters/agent/exporter.spec.js +++ b/packages/dd-trace/test/exporters/agent/exporter.spec.js @@ -15,11 +15,13 @@ describe('Exporter', () => { let writer let prioritySampler let span + let llmobs beforeEach(() => { url = 'www.example.com' flushInterval = 1000 span = {} + llmobs = {} writer = { append: sinon.spy(), flush: sinon.spy(), @@ -35,7 +37,7 @@ describe('Exporter', () => { it('should pass computed stats header through to writer', () => { const stats = { enabled: true } - exporter = new Exporter({ url, flushInterval, stats }, prioritySampler) + exporter = new Exporter({ url, flushInterval, stats, llmobs }, prioritySampler) expect(Writer).to.have.been.calledWithMatch({ headers: { 'Datadog-Client-Computed-Stats': 'yes' @@ -46,7 +48,7 @@ describe('Exporter', () => { it('should pass computed stats header through to writer if standalone appsec is enabled', () => { const stats = { enabled: false } const appsec = { standalone: { enabled: true } } - exporter = new Exporter({ url, flushInterval, stats, appsec }, prioritySampler) + exporter = new Exporter({ url, flushInterval, stats, appsec, llmobs }, prioritySampler) expect(Writer).to.have.been.calledWithMatch({ headers: { @@ -57,7 +59,7 @@ describe('Exporter', () => { it('should support IPv6', () => { const stats = { enabled: true } - exporter = new Exporter({ hostname: '::1', flushInterval, stats }, prioritySampler) + exporter = new Exporter({ hostname: '::1', flushInterval, stats, llmobs }, prioritySampler) expect(Writer).to.have.been.calledWithMatch({ url: new URL('http://[::1]') }) @@ -65,11 +67,11 @@ describe('Exporter', () => { describe('when interval is set to a positive number', () => { beforeEach(() => { - exporter = new Exporter({ url, flushInterval }, prioritySampler) + exporter = new Exporter({ url, flushInterval, llmobs }, prioritySampler) }) it('should not flush if export has not been called', (done) => { - exporter = new Exporter({ url, flushInterval }, prioritySampler) + exporter = new Exporter({ url, flushInterval, llmobs }, prioritySampler) setTimeout(() => { expect(writer.flush).not.to.have.been.called done() @@ -77,7 +79,7 @@ describe('Exporter', () => { }) it('should flush after the configured interval if a payload has been exported', (done) => { - exporter = new Exporter({ url, flushInterval }, prioritySampler) + exporter = new Exporter({ url, flushInterval, llmobs }, prioritySampler) exporter.export([{}]) setTimeout(() => { expect(writer.flush).to.have.been.called @@ -101,7 +103,7 @@ describe('Exporter', () => { describe('when interval is set to 0', () => { beforeEach(() => { - exporter = new Exporter({ url, flushInterval: 0 }) + exporter = new Exporter({ url, flushInterval: 0, llmobs }) }) it('should flush right away when interval is set to 0', () => { @@ -112,7 +114,7 @@ describe('Exporter', () => { describe('setUrl', () => { beforeEach(() => { - exporter = new Exporter({ url }) + exporter = new Exporter({ url, llmobs }) }) it('should set the URL on self and writer', () => { @@ -122,4 +124,22 @@ describe('Exporter', () => { expect(writer.setUrl).to.have.been.calledWith(url) }) }) + + describe('with llmobs agentless enabled', () => { + beforeEach(() => { + exporter = new Exporter({ + url, + llmobs: { + enabled: true, + agentlessEnabled: true + } + }) + }) + + it('does not write the span', () => { + exporter.export([span]) + + expect(writer.append).to.not.have.been.called + }) + }) }) diff --git a/packages/dd-trace/test/format.spec.js b/packages/dd-trace/test/format.spec.js index 846a02cd66a..b103ea323a6 100644 --- a/packages/dd-trace/test/format.spec.js +++ b/packages/dd-trace/test/format.spec.js @@ -577,5 +577,15 @@ describe('format', () => { expect(trace.metrics).to.have.property('_dd1.sr.eausr', 1) }) + + it('should skip formatting temporary _ml_obs.* tags', () => { + spanContext._tags['_ml_obs.meta.span.kind'] = 'llm' + spanContext._tags['_ml_obs.meta.ml_app'] = 'test' + + trace = format(span) + + expect(trace.meta['_ml_obs.meta.span.kind']).to.be.undefined + expect(trace.meta['_ml_obs.meta.ml_app']).to.be.undefined + }) }) }) diff --git a/packages/dd-trace/test/llmobs/span_processor.spec.js b/packages/dd-trace/test/llmobs/span_processor.spec.js new file mode 100644 index 00000000000..3bc65ded83c --- /dev/null +++ b/packages/dd-trace/test/llmobs/span_processor.spec.js @@ -0,0 +1,356 @@ +'use strict' + +const { expect } = require('chai') +const proxyquire = require('proxyquire') + +describe('span processor', () => { + let LLMObsSpanProcessor + let processor + let AgentlessWriter + let AgentProxyWriter + let writer + + beforeEach(() => { + writer = { + append: sinon.stub() + } + AgentlessWriter = sinon.stub().returns(writer) + AgentProxyWriter = sinon.stub().returns(writer) + + LLMObsSpanProcessor = proxyquire('../../src/llmobs/span_processor', { + './writers/spans/agentless': AgentlessWriter, + './writers/spans/agentProxy': AgentProxyWriter, + '../../../../package.json': { version: 'x.y.z' } + }) + }) + + describe('initialization', () => { + it('should not create a writer if llmobs is not enabled', () => { + processor = new LLMObsSpanProcessor({ llmobs: { enabled: false } }) + + expect(processor._writer).to.be.undefined + }) + + it('should create an agentless writer if agentless is enabled', () => { + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true, agentlessEnabled: true } }) + + expect(AgentlessWriter).to.have.been.calledOnce + }) + + it('should create an agent proxy writer if agentless is not enabled', () => { + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true, agentlessEnabled: false } }) + + expect(AgentProxyWriter).to.have.been.calledOnce + }) + }) + + describe('process', () => { + let span + + it('should do nothing if llmobs is not enabled', () => { + processor = new LLMObsSpanProcessor({ llmobs: { enabled: false } }) + + expect(() => processor.process(span)).not.to.throw() + }) + + it('should do nothing if the span is not an llm obs span', () => { + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + span = { context: () => ({ _tags: {} }) } + + expect(processor._writer.append).to.not.have.been.called + }) + + // it('should append to the writer if the span is an llm obs span', () => { + // processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + // processor._process = sinon.stub() + // span = { context: () => ({ _tags: { 'span.type': 'llm' } }) } + + // processor.process(span) + + // expect(processor._process).to.have.been.calledOnce + // expect(processor._writer.append).to.have.been.calledOnce + // }) + + it('should format the span event for the writer', () => { + span = { + _name: 'test', + _startTime: 0, // this is in ms, will be converted to ns + _duration: 1, // this is in ms, will be converted to ns + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.meta.model_name': 'myModel', + '_ml_obs.meta.model_provider': 'myProvider', + '_ml_obs.meta.metadata': JSON.stringify({ foo: 'bar' }), + '_ml_obs.meta.ml_app': 'myApp', + '_ml_obs.meta.input.value': 'input-value', + '_ml_obs.meta.output.value': 'output-value', + '_ml_obs.meta.input.messages': '{"role":"user","content":"hello"}', + '_ml_obs.meta.output.messages': '{"role":"assistant","content":"world"}', + '_ml_obs.llmobs_parent_id': '1234' + }, + toTraceId () { return '123' }, // should not use this + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload).to.deep.equal({ + trace_id: '123', + span_id: '456', + parent_id: '1234', + name: 'test', + tags: [ + 'version:', + 'env:', + 'service:', + 'source:integration', + 'ml_app:myApp', + 'dd-trace.version:x.y.z', + 'error:0', + 'language:javascript' + ], + start_ns: 0, + duration: 1000000, + status: 'ok', + meta: { + 'span.kind': 'llm', + model_name: 'myModel', + model_provider: 'myprovider', // should be lowercase + input: { + value: 'input-value', + messages: { role: 'user', content: 'hello' } + }, + output: { + value: 'output-value', + messages: { role: 'assistant', content: 'world' } + }, + metadata: { foo: 'bar' } + }, + metrics: {}, + _dd: { + trace_id: '123', + span_id: '456' + } + }) + }) + + it('tags output documents for a retrieval span', () => { + span = { + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'retrieval', + '_ml_obs.meta.output.documents': '[{"text":"hello","name":"myDoc","id":"1","score":0.6}]' + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.meta.output.documents).to.deep.equal([{ + text: 'hello', + name: 'myDoc', + id: '1', + score: 0.6 + }]) + }) + + it('tags input documents for an embedding span', () => { + span = { + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'embedding', + '_ml_obs.meta.input.documents': '[{"text":"hello","name":"myDoc","id":"1","score":0.6}]' + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.meta.input.documents).to.deep.equal([{ + text: 'hello', + name: 'myDoc', + id: '1', + score: 0.6 + }]) + }) + + it('defaults model provider to custom', () => { + span = { + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.meta.model_name': 'myModel' + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.meta.model_provider).to.equal('custom') + }) + + it('sets an error appropriately', () => { + span = { + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'llm', + error: new Error(), + 'error.message': 'error message', + 'error.type': 'error type', + 'error.stack': 'error stack' + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.meta['error.message']).to.equal('error message') + expect(payload.meta['error.type']).to.equal('error type') + expect(payload.meta['error.stack']).to.equal('error stack') + expect(payload.status).to.equal('error') + + expect(payload.tags).to.include('error_type:error type') + }) + + it('uses the error itself if the span does not have specific error fields', () => { + span = { + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'llm', + error: new Error('error message') + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.meta['error.message']).to.equal('error message') + expect(payload.meta['error.type']).to.equal('Error') + expect(payload.meta['error.stack']).to.exist + expect(payload.status).to.equal('error') + + expect(payload.tags).to.include('error_type:Error') + }) + + it('uses the span name from the tag if provided', () => { + span = { + _name: 'test', + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.name': 'mySpan' + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.name).to.equal('mySpan') + }) + + it('attaches session id if provided', () => { + span = { + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.session_id': '1234' + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.session_id).to.equal('1234') + expect(payload.tags).to.include('session_id:1234') + }) + + it('sets span tags appropriately', () => { + span = { + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.tags': '{"hostnam":"localhost","foo":"bar","source":"mySource"}' + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.tags).to.include('foo:bar') + expect(payload.tags).to.include('source:mySource') + expect(payload.tags).to.not.include('hostname:localhost') + }) + }) +}) diff --git a/packages/dd-trace/test/llmobs/util.spec.js b/packages/dd-trace/test/llmobs/util.spec.js index e365c01e13b..99537913072 100644 --- a/packages/dd-trace/test/llmobs/util.spec.js +++ b/packages/dd-trace/test/llmobs/util.spec.js @@ -1,7 +1,9 @@ 'use strict' +const { SPAN_TYPE } = require('../../../../ext/tags') const { - encodeUnicode + encodeUnicode, + isLLMSpan } = require('../../src/llmobs/util') describe('util', () => { @@ -14,4 +16,27 @@ describe('util', () => { expect(encodeUnicode('test 😀')).to.equal('test \\ud83d\\ude00') }) }) + + describe('isLLMSpan', () => { + it('should return false for an undefined span', () => { + expect(isLLMSpan(undefined)).to.equal(false) + }) + + it('should return false for a span without a SPAN_KIND tag', () => { + const span = { context: () => ({ _tags: {} }) } + expect(isLLMSpan(span)).to.equal(false) + }) + + it('should return false for a span with an invalid span type', () => { + const span = { context: () => ({ _tags: { [SPAN_TYPE]: 'invalid' } }) } + expect(isLLMSpan(span)).to.equal(false) + }) + + for (const spanType of ['llm', 'openai']) { + it(`should return true for a span with a valid span type: ${spanType}`, () => { + const span = { context: () => ({ _tags: { [SPAN_TYPE]: spanType } }) } + expect(isLLMSpan(span)).to.equal(true) + }) + } + }) }) diff --git a/packages/dd-trace/test/span_processor.spec.js b/packages/dd-trace/test/span_processor.spec.js index 5198e1702bc..0bf91cf20d6 100644 --- a/packages/dd-trace/test/span_processor.spec.js +++ b/packages/dd-trace/test/span_processor.spec.js @@ -15,6 +15,8 @@ describe('SpanProcessor', () => { let config let SpanSampler let sample + let LLMObsSpanProcessor + let LLMObsSpanProcessorInstance beforeEach(() => { tracer = {} @@ -45,6 +47,9 @@ describe('SpanProcessor', () => { flushMinSpans: 3, stats: { enabled: false + }, + llmobs: { + enabled: false } } format = sinon.stub().returns({ formatted: true }) @@ -54,9 +59,15 @@ describe('SpanProcessor', () => { sample }) + LLMObsSpanProcessorInstance = { + process: sinon.stub() + } + LLMObsSpanProcessor = sinon.stub().returns(LLMObsSpanProcessorInstance) + SpanProcessor = proxyquire('../src/span_processor', { './format': format, - './span_sampler': SpanSampler + './span_sampler': SpanSampler, + './llmobs/span_processor': LLMObsSpanProcessor }) processor = new SpanProcessor(exporter, prioritySampler, config) }) @@ -123,7 +134,8 @@ describe('SpanProcessor', () => { maxPerSecond: 456 } ] - } + }, + llmobs: { enabled: false } } const processor = new SpanProcessor(exporter, prioritySampler, config) @@ -137,6 +149,9 @@ describe('SpanProcessor', () => { tracing: false, stats: { enabled: false + }, + llmobs: { + enabled: false } } @@ -151,4 +166,24 @@ describe('SpanProcessor', () => { expect(finishedSpan.context()).to.have.deep.property('_tags', {}) expect(exporter.export).not.to.have.been.called }) + + it('should initialize and call the LLMObs span processor', () => { + const config = { + llmobs: { + enabled: true + }, + stats: { + enabled: false + } + } + + const processor = new SpanProcessor(exporter, prioritySampler, config) + trace.started = [finishedSpan] + trace.finished = [finishedSpan] + + processor.process(finishedSpan) + + expect(LLMObsSpanProcessor).to.have.been.calledWith(config) + expect(LLMObsSpanProcessorInstance.process).to.have.been.calledWith(finishedSpan) + }) }) From 1aae7c3e0b7a5e87c2cd963fe2fff2f1e6d6e1f2 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Wed, 2 Oct 2024 18:40:51 -0400 Subject: [PATCH 03/17] remove agent exporter log and do not stringify tags --- .../dd-trace/src/exporters/agent/index.js | 6 - .../dd-trace/src/llmobs/span_processor.js | 64 ++++- packages/dd-trace/src/llmobs/tagger.js | 237 +++++++++--------- .../test/exporters/agent/exporter.spec.js | 18 -- .../test/llmobs/span_processor.spec.js | 73 ++++-- packages/dd-trace/test/llmobs/tagger.spec.js | 94 ++++--- 6 files changed, 277 insertions(+), 215 deletions(-) diff --git a/packages/dd-trace/src/exporters/agent/index.js b/packages/dd-trace/src/exporters/agent/index.js index 58dd7b35b95..b2f25eeda99 100644 --- a/packages/dd-trace/src/exporters/agent/index.js +++ b/packages/dd-trace/src/exporters/agent/index.js @@ -42,12 +42,6 @@ class AgentExporter { } export (spans) { - const { llmobs } = this._config - if (llmobs.enabled && llmobs.agentlessEnabled) { - log.debug('LLMObs agentless mode enabled. Not sending APM spans to the agent.') - return - } - this._writer.append(spans) const { flushInterval } = this._config diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js index 4724f64334c..998e752db55 100644 --- a/packages/dd-trace/src/llmobs/span_processor.js +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -30,6 +30,7 @@ const AgentProxyWriter = require('./writers/spans/agentProxy') const { isLLMSpan } = require('./util') const tracerVersion = require('../../../../package.json').version +const logger = require('../log') class LLMObsSpanProcessor { constructor (config) { @@ -45,12 +46,22 @@ class LLMObsSpanProcessor { process (span) { if (!this._config.llmobs.enabled) return if (!isLLMSpan(span)) return - const payload = this._process(span) - - this._writer.append(payload) + const formattedEvent = this.format(span) + + try { + this._writer.append(formattedEvent) + } catch (e) { + // this should be a rare case + // we protect against unserializable properties in the format function, and in + // safeguards in the tagger + logger.warn(` + Failed to append span to LLM Observability writer, likely due to an unserializable property. + Span won't be sent to LLM Observability: ${e.message} + `) + } } - _process (span) { + format (span) { const tags = span.context()._tags const spanKind = tags[SPAN_KIND] @@ -63,25 +74,25 @@ class LLMObsSpanProcessor { meta.model_provider = (tags[MODEL_PROVIDER] || 'custom').toLowerCase() } if (tags[METADATA]) { - meta.metadata = JSON.parse(tags[METADATA]) + this._addObject(tags[METADATA], meta.metadata = {}) } if (spanKind === 'llm' && tags[INPUT_MESSAGES]) { - input.messages = JSON.parse(tags[INPUT_MESSAGES]) + input.messages = tags[INPUT_MESSAGES] } if (tags[INPUT_VALUE]) { input.value = tags[INPUT_VALUE] } if (spanKind === 'llm' && tags[OUTPUT_MESSAGES]) { - output.messages = JSON.parse(tags[OUTPUT_MESSAGES]) + output.messages = tags[OUTPUT_MESSAGES] } if (spanKind === 'embedding' && tags[INPUT_DOCUMENTS]) { - input.documents = JSON.parse(tags[INPUT_DOCUMENTS]) + input.documents = tags[INPUT_DOCUMENTS] } if (tags[OUTPUT_VALUE]) { output.value = tags[OUTPUT_VALUE] } if (spanKind === 'retrieval' && tags[OUTPUT_DOCUMENTS]) { - output.documents = JSON.parse(tags[OUTPUT_DOCUMENTS]) + output.documents = tags[OUTPUT_DOCUMENTS] } const error = tags.error @@ -94,7 +105,7 @@ class LLMObsSpanProcessor { if (input) meta.input = input if (output) meta.output = output - const metrics = JSON.parse(tags[METRICS] || '{}') + const metrics = tags[METRICS] || {} const mlApp = tags[ML_APP] const sessionId = tags[SESSION_ID] @@ -124,6 +135,37 @@ class LLMObsSpanProcessor { return llmObsSpanEvent } + // For now, this only applies to metadata, as we let users annotate this field with any object + // However, we want to protect against circular references or BigInts (unserializable) + // This function can be reused for other fields if needed + // Messages, Documents, and Metrics are safeguarded in `llmobs/tagger.js` + _addObject (obj, carrier) { + const seenObjects = new WeakSet() + seenObjects.add(obj) // capture root object + + const isCircular = value => { + if (typeof value !== 'object') return false + if (seenObjects.has(value)) return true + seenObjects.add(value) + return false + } + + const add = (obj, carrier) => { + for (const key in obj) { + const value = obj[key] + if (!Object.prototype.hasOwnProperty.call(obj, key)) continue + if (typeof value === 'bigint' || isCircular(value)) continue + if (typeof value === 'object') { + add(value, carrier[key] = {}) + } else { + carrier[key] = value + } + } + } + + add(obj, carrier) + } + _processTags (span, mlApp, sessionId, error) { let tags = { version: this._config.version, @@ -138,7 +180,7 @@ class LLMObsSpanProcessor { const errType = span.context()._tags[ERROR_TYPE] || error?.name if (errType) tags.error_type = errType if (sessionId) tags.session_id = sessionId - const existingTags = JSON.parse(span.context()._tags[TAGS] || '{}') + const existingTags = span.context()._tags[TAGS] || {} if (existingTags) tags = { ...tags, ...existingTags } return Object.entries(tags).map(([key, value]) => `${key}:${value ?? ''}`) } diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index 7c59ef9241e..8efef845968 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -78,32 +78,29 @@ class LLMObsTagger { } tagMetadata (span, metadata) { - try { - span.setTag(METADATA, JSON.stringify(metadata)) - } catch { - logger.warn('Failed to parse span metadata. Metadata key-value pairs must be JSON serializable.') - } + span.setTag(METADATA, metadata) } tagMetrics (span, metrics) { - try { - span.setTag(METRICS, JSON.stringify(metrics)) - } catch { - logger.warn('Failed to parse span metrics. Metrics key-value pairs must be JSON serializable.') + const filterdMetrics = {} + for (const [key, value] of Object.entries(metrics)) { + if (typeof value === 'number') { + filterdMetrics[key] = value + } else { + logger.warn(`Metric ${key} must be a number, instead got ${value}`) + } } + + span.setTag(METRICS, filterdMetrics) } tagSpanTags (span, tags) { // new tags will be merged with existing tags - try { - const currentTags = span.context()._tags[TAGS] - if (currentTags) { - Object.assign(tags, JSON.parse(currentTags)) - } - span.setTag(TAGS, JSON.stringify(tags)) - } catch { - logger.warn('Failed to parse span tags. Tag key-value pairs must be JSON serializable.') + const currentTags = span.context()._tags[TAGS] + if (currentTags) { + Object.assign(tags, currentTags) } + span.setTag(TAGS, tags) } _tagText (span, data, key) { @@ -127,58 +124,53 @@ class LLMObsTagger { data = [data] } - try { - const documents = data.map(document => { - if (typeof document === 'string') { - return { text: document } - } + const documents = data.map(document => { + if (typeof document === 'string') { + return { text: document } + } - if (document == null || typeof document !== 'object') { - logger.warn('Documents must be a string, object, or list of objects.') - return undefined - } + if (document == null || typeof document !== 'object') { + logger.warn('Documents must be a string, object, or list of objects.') + return undefined + } - const { text, name, id, score } = document + const { text, name, id, score } = document - if (typeof text !== 'string') { - logger.warn('Document text must be a string.') - return undefined - } + if (typeof text !== 'string') { + logger.warn('Document text must be a string.') + return undefined + } - const documentObj = { text } + const documentObj = { text } - if (name) { - if (typeof name !== 'string') { - logger.warn('Document name must be a string.') - return undefined - } - documentObj.name = name + if (name) { + if (typeof name !== 'string') { + logger.warn('Document name must be a string.') + return undefined } + documentObj.name = name + } - if (id) { - if (typeof id !== 'string') { - logger.warn('Document ID must be a string.') - return undefined - } - documentObj.id = id + if (id) { + if (typeof id !== 'string') { + logger.warn('Document ID must be a string.') + return undefined } + documentObj.id = id + } - if (score) { - if (typeof score !== 'number') { - logger.warn('Document score must be a number.') - return undefined - } - documentObj.score = score + if (score) { + if (typeof score !== 'number') { + logger.warn('Document score must be a number.') + return undefined } + documentObj.score = score + } - return documentObj - }).filter(doc => !!doc) + return documentObj + }).filter(doc => !!doc) - span.setTag(key, JSON.stringify(documents)) - } catch { - const type = key === INPUT_DOCUMENTS ? 'input' : 'output' - logger.warn(`Failed to parse ${type} documents.`) - } + span.setTag(key, documents) } } @@ -188,97 +180,92 @@ class LLMObsTagger { data = [data] } - try { - const messages = data.map(message => { - if (typeof message === 'string') { - return { content: message } - } + const messages = data.map(message => { + if (typeof message === 'string') { + return { content: message } + } - if (message == null || typeof message !== 'object') { - logger.warn('Messages must be a string, object, or list of objects') - return undefined - } + if (message == null || typeof message !== 'object') { + logger.warn('Messages must be a string, object, or list of objects') + return undefined + } + + const { content = '', role } = message + let toolCalls = message.toolCalls + const messageObj = { content } - const { content = '', role } = message - let toolCalls = message.toolCalls - const messageObj = { content } + if (typeof content !== 'string') { + logger.warn('Message content must be a string.') + return undefined + } - if (typeof content !== 'string') { - logger.warn('Message content must be a string.') + if (role) { + if (typeof role !== 'string') { + logger.warn('Message role must be a string.') return undefined } + messageObj.role = role + } - if (role) { - if (typeof role !== 'string') { - logger.warn('Message role must be a string.') - return undefined - } - messageObj.role = role + if (toolCalls) { + if (!Array.isArray(toolCalls)) { + toolCalls = [toolCalls] } - if (toolCalls) { - if (!Array.isArray(toolCalls)) { - toolCalls = [toolCalls] + const filteredToolCalls = toolCalls.map(toolCall => { + if (typeof toolCall !== 'object') { + logger.warn('Tool call must be an object.') + return undefined } - const filteredToolCalls = toolCalls.map(toolCall => { - if (typeof toolCall !== 'object') { - logger.warn('Tool call must be an object.') - return undefined - } + const { name, arguments: args, toolId, type } = toolCall + const toolCallObj = {} - const { name, arguments: args, toolId, type } = toolCall - const toolCallObj = {} - - if (name) { - if (typeof name !== 'string') { - logger.warn('Tool name must be a string.') - return undefined - } - toolCallObj.name = name + if (name) { + if (typeof name !== 'string') { + logger.warn('Tool name must be a string.') + return undefined } + toolCallObj.name = name + } - if (args) { - if (typeof args !== 'object') { - logger.warn('Tool arguments must be an object.') - return undefined - } - toolCallObj.arguments = args + if (args) { + if (typeof args !== 'object') { + logger.warn('Tool arguments must be an object.') + return undefined } + toolCallObj.arguments = args + } - if (toolId) { - if (typeof toolId !== 'string') { - logger.warn('Tool ID must be a string.') - return undefined - } - toolCallObj.toolId = toolId + if (toolId) { + if (typeof toolId !== 'string') { + logger.warn('Tool ID must be a string.') + return undefined } + toolCallObj.toolId = toolId + } - if (type) { - if (typeof type !== 'string') { - logger.warn('Tool type must be a string.') - return undefined - } - toolCallObj.type = type + if (type) { + if (typeof type !== 'string') { + logger.warn('Tool type must be a string.') + return undefined } + toolCallObj.type = type + } - return toolCallObj - }).filter(toolCall => !!toolCall) + return toolCallObj + }).filter(toolCall => !!toolCall) - if (filteredToolCalls.length) { - messageObj.tool_calls = filteredToolCalls - } + if (filteredToolCalls.length) { + messageObj.tool_calls = filteredToolCalls } + } - return messageObj - }).filter(msg => !!msg) + return messageObj + }).filter(msg => !!msg) - if (messages.length) { - span.setTag(key, JSON.stringify(messages)) - } - } catch { - const type = key === INPUT_MESSAGES ? 'input' : 'output' - logger.warn(`Failed to parse ${type} messages.`) + if (messages.length) { + span.setTag(key, messages) } } } diff --git a/packages/dd-trace/test/exporters/agent/exporter.spec.js b/packages/dd-trace/test/exporters/agent/exporter.spec.js index 67ba988ce67..1ca1998d666 100644 --- a/packages/dd-trace/test/exporters/agent/exporter.spec.js +++ b/packages/dd-trace/test/exporters/agent/exporter.spec.js @@ -124,22 +124,4 @@ describe('Exporter', () => { expect(writer.setUrl).to.have.been.calledWith(url) }) }) - - describe('with llmobs agentless enabled', () => { - beforeEach(() => { - exporter = new Exporter({ - url, - llmobs: { - enabled: true, - agentlessEnabled: true - } - }) - }) - - it('does not write the span', () => { - exporter.export([span]) - - expect(writer.append).to.not.have.been.called - }) - }) }) diff --git a/packages/dd-trace/test/llmobs/span_processor.spec.js b/packages/dd-trace/test/llmobs/span_processor.spec.js index 3bc65ded83c..666c8101f64 100644 --- a/packages/dd-trace/test/llmobs/span_processor.spec.js +++ b/packages/dd-trace/test/llmobs/span_processor.spec.js @@ -9,6 +9,7 @@ describe('span processor', () => { let AgentlessWriter let AgentProxyWriter let writer + let log beforeEach(() => { writer = { @@ -16,11 +17,15 @@ describe('span processor', () => { } AgentlessWriter = sinon.stub().returns(writer) AgentProxyWriter = sinon.stub().returns(writer) + log = { + warn: sinon.stub() + } LLMObsSpanProcessor = proxyquire('../../src/llmobs/span_processor', { './writers/spans/agentless': AgentlessWriter, './writers/spans/agentProxy': AgentProxyWriter, - '../../../../package.json': { version: 'x.y.z' } + '../../../../package.json': { version: 'x.y.z' }, + '../log': log }) }) @@ -60,17 +65,6 @@ describe('span processor', () => { expect(processor._writer.append).to.not.have.been.called }) - // it('should append to the writer if the span is an llm obs span', () => { - // processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - // processor._process = sinon.stub() - // span = { context: () => ({ _tags: { 'span.type': 'llm' } }) } - - // processor.process(span) - - // expect(processor._process).to.have.been.calledOnce - // expect(processor._writer.append).to.have.been.calledOnce - // }) - it('should format the span event for the writer', () => { span = { _name: 'test', @@ -83,12 +77,12 @@ describe('span processor', () => { '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.model_name': 'myModel', '_ml_obs.meta.model_provider': 'myProvider', - '_ml_obs.meta.metadata': JSON.stringify({ foo: 'bar' }), + '_ml_obs.meta.metadata': { foo: 'bar' }, '_ml_obs.meta.ml_app': 'myApp', '_ml_obs.meta.input.value': 'input-value', '_ml_obs.meta.output.value': 'output-value', - '_ml_obs.meta.input.messages': '{"role":"user","content":"hello"}', - '_ml_obs.meta.output.messages': '{"role":"assistant","content":"world"}', + '_ml_obs.meta.input.messages': [{ role: 'user', content: 'hello' }], + '_ml_obs.meta.output.messages': [{ role: 'assistant', content: 'world' }], '_ml_obs.llmobs_parent_id': '1234' }, toTraceId () { return '123' }, // should not use this @@ -126,11 +120,11 @@ describe('span processor', () => { model_provider: 'myprovider', // should be lowercase input: { value: 'input-value', - messages: { role: 'user', content: 'hello' } + messages: [{ role: 'user', content: 'hello' }] }, output: { value: 'output-value', - messages: { role: 'assistant', content: 'world' } + messages: [{ role: 'assistant', content: 'world' }] }, metadata: { foo: 'bar' } }, @@ -140,6 +134,43 @@ describe('span processor', () => { span_id: '456' } }) + + expect(writer.append).to.have.been.calledOnce + }) + + it('removes problematic fields from the metadata', () => { + // problematic fields are circular references or bigints + const metadata = { + bigint: BigInt(1), + deep: { + foo: 'bar' + }, + bar: 'baz' + } + metadata.circular = metadata + metadata.deep.circular = metadata.deep + span = { + context () { + return { + _tags: { + 'span.type': 'llm', + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.meta.metadata': metadata + }, + toTraceId () { return '123' }, + toSpanId () { return '456' } + } + } + } + + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) + processor.process(span) + const payload = writer.append.getCall(0).firstArg + + expect(payload.meta.metadata).to.deep.equal({ + bar: 'baz', + deep: { foo: 'bar' } + }) }) it('tags output documents for a retrieval span', () => { @@ -149,7 +180,7 @@ describe('span processor', () => { _tags: { 'span.type': 'llm', '_ml_obs.meta.span.kind': 'retrieval', - '_ml_obs.meta.output.documents': '[{"text":"hello","name":"myDoc","id":"1","score":0.6}]' + '_ml_obs.meta.output.documents': [{ text: 'hello', name: 'myDoc', id: '1', score: 0.6 }] }, toTraceId () { return '123' }, toSpanId () { return '456' } @@ -177,7 +208,7 @@ describe('span processor', () => { _tags: { 'span.type': 'llm', '_ml_obs.meta.span.kind': 'embedding', - '_ml_obs.meta.input.documents': '[{"text":"hello","name":"myDoc","id":"1","score":0.6}]' + '_ml_obs.meta.input.documents': [{ text: 'hello', name: 'myDoc', id: '1', score: 0.6 }] }, toTraceId () { return '123' }, toSpanId () { return '456' } @@ -335,7 +366,7 @@ describe('span processor', () => { _tags: { 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', - '_ml_obs.tags': '{"hostnam":"localhost","foo":"bar","source":"mySource"}' + '_ml_obs.tags': { hostname: 'localhost', foo: 'bar', source: 'mySource' } }, toTraceId () { return '123' }, toSpanId () { return '456' } @@ -350,7 +381,7 @@ describe('span processor', () => { expect(payload.tags).to.include('foo:bar') expect(payload.tags).to.include('source:mySource') - expect(payload.tags).to.not.include('hostname:localhost') + expect(payload.tags).to.include('hostname:localhost') }) }) }) diff --git a/packages/dd-trace/test/llmobs/tagger.spec.js b/packages/dd-trace/test/llmobs/tagger.spec.js index 935389cd985..35768ffbb7b 100644 --- a/packages/dd-trace/test/llmobs/tagger.spec.js +++ b/packages/dd-trace/test/llmobs/tagger.spec.js @@ -165,29 +165,32 @@ describe('tagger', () => { it('tags a span with metadata', () => { tagger.tagMetadata(span, { a: 'foo', b: 'bar' }) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.meta.metadata': '{"a":"foo","b":"bar"}' + '_ml_obs.meta.metadata': { a: 'foo', b: 'bar' } }) }) - - it('logs when metadata is not JSON serializable', () => { - const metadata = unserializbleObject() - tagger.tagMetadata(span, metadata) - expect(logger.warn).to.have.been.calledOnce - }) }) describe('tagMetrics', () => { it('tags a span with metrics', () => { tagger.tagMetadata(span, { a: 1, b: 2 }) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.meta.metadata': '{"a":1,"b":2}' + '_ml_obs.meta.metadata': { a: 1, b: 2 } }) }) - it('logs when metrics is not JSON serializable', () => { - const metadata = unserializbleObject() - tagger.tagMetadata(span, metadata) - expect(logger.warn).to.have.been.calledOnce + it('removes non-number entries', () => { + const metrics = { + a: 1, + b: 'foo', + c: { depth: 1 }, + d: undefined + } + tagger.tagMetrics(span, metrics) + expect(span.context()._tags).to.deep.equal({ + '_ml_obs.metrics': { a: 1 } + }) + + expect(logger.warn).to.have.been.calledThrice }) }) @@ -196,16 +199,16 @@ describe('tagger', () => { const tags = { foo: 'bar' } tagger.tagSpanTags(span, tags) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.tags': '{"foo":"bar"}' + '_ml_obs.tags': { foo: 'bar' } }) }) it('merges tags so they do not overwrite', () => { - span.context()._tags['_ml_obs.tags'] = '{"a":1}' + span.context()._tags['_ml_obs.tags'] = { a: 1 } const tags = { a: 2, b: 1 } tagger.tagSpanTags(span, tags) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.tags': '{"a":1,"b":1}' + '_ml_obs.tags': { a: 1, b: 1 } }) }) }) @@ -224,10 +227,14 @@ describe('tagger', () => { tagger.tagLLMIO(span, inputData, outputData) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.meta.input.messages': '[{"content":"you are an amazing assistant"},' + - '{"content":"hello! my name is foobar"},{"content":"I am a robot","role":"assistant"},' + - '{"content":"I am a human","role":"user"},{"content":""}]', - '_ml_obs.meta.output.messages': '[{"content":"Nice to meet you, human!"}]' + '_ml_obs.meta.input.messages': [ + { content: 'you are an amazing assistant' }, + { content: 'hello! my name is foobar' }, + { content: 'I am a robot', role: 'assistant' }, + { content: 'I am a human', role: 'user' }, + { content: '' } + ], + '_ml_obs.meta.output.messages': [{ content: 'Nice to meet you, human!' }] }) }) @@ -246,7 +253,7 @@ describe('tagger', () => { ] tagger.tagLLMIO(span, inputData, outputData) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.meta.input.messages': '[{"content":"hi"}]' + '_ml_obs.meta.input.messages': [{ content: 'hi' }] }) expect(logger.warn.getCall(0).firstArg).to.equal('Messages must be a string, object, or list of objects') @@ -270,10 +277,15 @@ describe('tagger', () => { tagger.tagLLMIO(span, inputData, outputData) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.meta.input.messages': '[{"content":"hello","tool_calls":[{"name":"tool1"},' + - '{"name":"tool2","arguments":{"a":1,"b":2}}]},' + - '{"content":"goodbye","tool_calls":[{"name":"tool3"}]}]', - '_ml_obs.meta.output.messages': '[{"content":"hi","tool_calls":[{"name":"tool4"}]}]' + '_ml_obs.meta.input.messages': [ + { + content: 'hello', + tool_calls: [{ name: 'tool1' }, { name: 'tool2', arguments: { a: 1, b: 2 } }] + }, { + content: 'goodbye', + tool_calls: [{ name: 'tool3' }] + }], + '_ml_obs.meta.output.messages': [{ content: 'hi', tool_calls: [{ name: 'tool4' }] }] }) }) @@ -295,8 +307,14 @@ describe('tagger', () => { tagger.tagLLMIO(span, inputData, undefined) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.meta.input.messages': '[{"content":"a"},{"content":"b"},{"content":"c"},' + - '{"content":"d"},{"content":"e"},{"content":"f"},{"content":"g","tool_calls":[{"name":"tool2"}]}]' + '_ml_obs.meta.input.messages': [ + { content: 'a' }, + { content: 'b' }, + { content: 'c' }, + { content: 'd' }, + { content: 'e' }, + { content: 'f' }, + { content: 'g', tool_calls: [{ name: 'tool2' }] }] }) expect(logger.warn.getCall(0).firstArg).to.equal('Tool call must be an object.') @@ -323,9 +341,13 @@ describe('tagger', () => { const outputData = 'embedded documents' tagger.tagEmbeddingIO(span, inputData, outputData) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.meta.input.documents': '[{"text":"my string document"},{"text":"my object document"},' + - '{"text":"foo","name":"bar"},{"text":"baz","id":"qux"},{"text":"quux","score":5},' + - '{"text":"foo","name":"bar","id":"qux","score":5}]', + '_ml_obs.meta.input.documents': [ + { text: 'my string document' }, + { text: 'my object document' }, + { text: 'foo', name: 'bar' }, + { text: 'baz', id: 'qux' }, + { text: 'quux', score: 5 }, + { text: 'foo', name: 'bar', id: 'qux', score: 5 }], '_ml_obs.meta.output.value': 'embedded documents' }) }) @@ -342,7 +364,7 @@ describe('tagger', () => { const outputData = 'output' tagger.tagEmbeddingIO(span, inputData, outputData) expect(span.context()._tags).to.deep.equal({ - '_ml_obs.meta.input.documents': '[{"text":"hi"}]', + '_ml_obs.meta.input.documents': [{ text: 'hi' }], '_ml_obs.meta.output.value': 'output' }) @@ -369,9 +391,13 @@ describe('tagger', () => { tagger.tagRetrievalIO(span, inputData, outputData) expect(span.context()._tags).to.deep.equal({ '_ml_obs.meta.input.value': 'some query', - '_ml_obs.meta.output.documents': '[{"text":"result 1"},{"text":"result 2"},' + - '{"text":"foo","name":"bar"},{"text":"baz","id":"qux"},{"text":"quux","score":5},' + - '{"text":"foo","name":"bar","id":"qux","score":5}]' + '_ml_obs.meta.output.documents': [ + { text: 'result 1' }, + { text: 'result 2' }, + { text: 'foo', name: 'bar' }, + { text: 'baz', id: 'qux' }, + { text: 'quux', score: 5 }, + { text: 'foo', name: 'bar', id: 'qux', score: 5 }] }) }) @@ -388,7 +414,7 @@ describe('tagger', () => { tagger.tagRetrievalIO(span, inputData, outputData) expect(span.context()._tags).to.deep.equal({ '_ml_obs.meta.input.value': 'some query', - '_ml_obs.meta.output.documents': '[{"text":"hi"}]' + '_ml_obs.meta.output.documents': [{ text: 'hi' }] }) expect(logger.warn.getCall(0).firstArg).to.equal('Documents must be a string, object, or list of objects.') From 62a9cc9c7f20ac77c6d1250a2764e55ef2c64f7d Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Wed, 2 Oct 2024 19:10:03 -0400 Subject: [PATCH 04/17] remove llmobs from exporter tests --- .../test/exporters/agent/exporter.spec.js | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/dd-trace/test/exporters/agent/exporter.spec.js b/packages/dd-trace/test/exporters/agent/exporter.spec.js index 1ca1998d666..a3e402ba358 100644 --- a/packages/dd-trace/test/exporters/agent/exporter.spec.js +++ b/packages/dd-trace/test/exporters/agent/exporter.spec.js @@ -15,13 +15,11 @@ describe('Exporter', () => { let writer let prioritySampler let span - let llmobs beforeEach(() => { url = 'www.example.com' flushInterval = 1000 span = {} - llmobs = {} writer = { append: sinon.spy(), flush: sinon.spy(), @@ -37,7 +35,7 @@ describe('Exporter', () => { it('should pass computed stats header through to writer', () => { const stats = { enabled: true } - exporter = new Exporter({ url, flushInterval, stats, llmobs }, prioritySampler) + exporter = new Exporter({ url, flushInterval, stats }, prioritySampler) expect(Writer).to.have.been.calledWithMatch({ headers: { 'Datadog-Client-Computed-Stats': 'yes' @@ -48,7 +46,7 @@ describe('Exporter', () => { it('should pass computed stats header through to writer if standalone appsec is enabled', () => { const stats = { enabled: false } const appsec = { standalone: { enabled: true } } - exporter = new Exporter({ url, flushInterval, stats, appsec, llmobs }, prioritySampler) + exporter = new Exporter({ url, flushInterval, stats, appsec }, prioritySampler) expect(Writer).to.have.been.calledWithMatch({ headers: { @@ -59,7 +57,7 @@ describe('Exporter', () => { it('should support IPv6', () => { const stats = { enabled: true } - exporter = new Exporter({ hostname: '::1', flushInterval, stats, llmobs }, prioritySampler) + exporter = new Exporter({ hostname: '::1', flushInterval, stats }, prioritySampler) expect(Writer).to.have.been.calledWithMatch({ url: new URL('http://[::1]') }) @@ -67,11 +65,11 @@ describe('Exporter', () => { describe('when interval is set to a positive number', () => { beforeEach(() => { - exporter = new Exporter({ url, flushInterval, llmobs }, prioritySampler) + exporter = new Exporter({ url, flushInterval }, prioritySampler) }) it('should not flush if export has not been called', (done) => { - exporter = new Exporter({ url, flushInterval, llmobs }, prioritySampler) + exporter = new Exporter({ url, flushInterval }, prioritySampler) setTimeout(() => { expect(writer.flush).not.to.have.been.called done() @@ -79,7 +77,7 @@ describe('Exporter', () => { }) it('should flush after the configured interval if a payload has been exported', (done) => { - exporter = new Exporter({ url, flushInterval, llmobs }, prioritySampler) + exporter = new Exporter({ url, flushInterval }, prioritySampler) exporter.export([{}]) setTimeout(() => { expect(writer.flush).to.have.been.called @@ -103,7 +101,7 @@ describe('Exporter', () => { describe('when interval is set to 0', () => { beforeEach(() => { - exporter = new Exporter({ url, flushInterval: 0, llmobs }) + exporter = new Exporter({ url, flushInterval: 0 }) }) it('should flush right away when interval is set to 0', () => { @@ -114,7 +112,7 @@ describe('Exporter', () => { describe('setUrl', () => { beforeEach(() => { - exporter = new Exporter({ url, llmobs }) + exporter = new Exporter({ url }) }) it('should set the URL on self and writer', () => { From 53c1d1adddb2bac8660275348313062c860a6722 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Thu, 3 Oct 2024 14:08:17 -0400 Subject: [PATCH 05/17] add in default unserializable value --- packages/dd-trace/src/llmobs/constants.js | 3 ++- packages/dd-trace/src/llmobs/span_processor.js | 10 ++++++++-- packages/dd-trace/test/llmobs/span_processor.spec.js | 4 +++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/src/llmobs/constants.js b/packages/dd-trace/src/llmobs/constants.js index 4cc63a8d01d..68d426e3745 100644 --- a/packages/dd-trace/src/llmobs/constants.js +++ b/packages/dd-trace/src/llmobs/constants.js @@ -36,5 +36,6 @@ module.exports = { EVP_EVENT_SIZE_LIMIT: (1 << 20) - 1024, // 999KB (actual limit is 1MB) DROPPED_IO_COLLECTION_ERROR: 'dropped_io', - DROPPED_VALUE_TEXT: "[This value has been dropped because this span's size exceeds the 1MB size limit.]" + DROPPED_VALUE_TEXT: "[This value has been dropped because this span's size exceeds the 1MB size limit.]", + UNSERIALIZABLE_VALUE_TEXT: 'Unserializable value' } diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js index 998e752db55..d34d322d5f0 100644 --- a/packages/dd-trace/src/llmobs/span_processor.js +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -16,7 +16,8 @@ const { TAGS, PARENT_ID_KEY, SESSION_ID, - NAME + NAME, + UNSERIALIZABLE_VALUE_TEXT } = require('./constants') const { @@ -154,7 +155,12 @@ class LLMObsSpanProcessor { for (const key in obj) { const value = obj[key] if (!Object.prototype.hasOwnProperty.call(obj, key)) continue - if (typeof value === 'bigint' || isCircular(value)) continue + if (typeof value === 'bigint' || isCircular(value)) { + // mark as unserializable instead of dropping + logger.warn(`Unserializable property found in metadata: ${key}`) + carrier[key] = UNSERIALIZABLE_VALUE_TEXT + continue + } if (typeof value === 'object') { add(value, carrier[key] = {}) } else { diff --git a/packages/dd-trace/test/llmobs/span_processor.spec.js b/packages/dd-trace/test/llmobs/span_processor.spec.js index 666c8101f64..e6388ec2fe2 100644 --- a/packages/dd-trace/test/llmobs/span_processor.spec.js +++ b/packages/dd-trace/test/llmobs/span_processor.spec.js @@ -169,7 +169,9 @@ describe('span processor', () => { expect(payload.meta.metadata).to.deep.equal({ bar: 'baz', - deep: { foo: 'bar' } + bigint: 'Unserializable value', + circular: 'Unserializable value', + deep: { foo: 'bar', circular: 'Unserializable value' } }) }) From 792054a9b02d3006ff4cfe38d68ac2bc0a2533a7 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Thu, 3 Oct 2024 16:32:19 -0400 Subject: [PATCH 06/17] review comments --- packages/dd-trace/src/format.js | 4 +++- packages/dd-trace/src/llmobs/span_processor.js | 4 ++-- packages/dd-trace/src/llmobs/tagger.js | 2 +- packages/dd-trace/src/llmobs/util.js | 4 ++-- packages/dd-trace/test/llmobs/util.spec.js | 10 +++++----- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/dd-trace/src/format.js b/packages/dd-trace/src/format.js index 230a04fbc9c..acdf96eaadf 100644 --- a/packages/dd-trace/src/format.js +++ b/packages/dd-trace/src/format.js @@ -30,6 +30,8 @@ const map = { 'resource.name': 'resource' } +const llmObsTagPrefix = '_ml_obs' + function format (span) { const formatted = formatSpan(span) @@ -160,7 +162,7 @@ function extractTags (trace, span) { break } default: // eslint-disable-line no-fallthrough - if (!tag.startsWith('_ml_obs')) { // don't add ml_obs-related tags + if (!tag.startsWith(llmObsTagPrefix)) { // don't add ml_obs-related tags addTag(trace.meta, trace.metrics, tag, tags[tag]) } } diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js index d34d322d5f0..e6779bbc80f 100644 --- a/packages/dd-trace/src/llmobs/span_processor.js +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -28,7 +28,7 @@ const { const AgentlessWriter = require('./writers/spans/agentless') const AgentProxyWriter = require('./writers/spans/agentProxy') -const { isLLMSpan } = require('./util') +const { isLLMObsSpan } = require('./util') const tracerVersion = require('../../../../package.json').version const logger = require('../log') @@ -46,7 +46,7 @@ class LLMObsSpanProcessor { process (span) { if (!this._config.llmobs.enabled) return - if (!isLLMSpan(span)) return + if (!isLLMObsSpan(span)) return const formattedEvent = this.format(span) try { diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index 8efef845968..673c5cd53c3 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -87,7 +87,7 @@ class LLMObsTagger { if (typeof value === 'number') { filterdMetrics[key] = value } else { - logger.warn(`Metric ${key} must be a number, instead got ${value}`) + logger.warn(`Metric "${key}" must be a number, instead got ${value}`) } } diff --git a/packages/dd-trace/src/llmobs/util.js b/packages/dd-trace/src/llmobs/util.js index c6463346eeb..ae31b6f7adb 100644 --- a/packages/dd-trace/src/llmobs/util.js +++ b/packages/dd-trace/src/llmobs/util.js @@ -13,11 +13,11 @@ function encodeUnicode (str) { }).join('') } -function isLLMSpan (span) { +function isLLMObsSpan (span) { return ['llm', 'openai'].includes(span?.context()._tags[SPAN_TYPE]) } module.exports = { encodeUnicode, - isLLMSpan + isLLMObsSpan } diff --git a/packages/dd-trace/test/llmobs/util.spec.js b/packages/dd-trace/test/llmobs/util.spec.js index 99537913072..987004dc8b8 100644 --- a/packages/dd-trace/test/llmobs/util.spec.js +++ b/packages/dd-trace/test/llmobs/util.spec.js @@ -3,7 +3,7 @@ const { SPAN_TYPE } = require('../../../../ext/tags') const { encodeUnicode, - isLLMSpan + isLLMObsSpan } = require('../../src/llmobs/util') describe('util', () => { @@ -19,23 +19,23 @@ describe('util', () => { describe('isLLMSpan', () => { it('should return false for an undefined span', () => { - expect(isLLMSpan(undefined)).to.equal(false) + expect(isLLMObsSpan(undefined)).to.equal(false) }) it('should return false for a span without a SPAN_KIND tag', () => { const span = { context: () => ({ _tags: {} }) } - expect(isLLMSpan(span)).to.equal(false) + expect(isLLMObsSpan(span)).to.equal(false) }) it('should return false for a span with an invalid span type', () => { const span = { context: () => ({ _tags: { [SPAN_TYPE]: 'invalid' } }) } - expect(isLLMSpan(span)).to.equal(false) + expect(isLLMObsSpan(span)).to.equal(false) }) for (const spanType of ['llm', 'openai']) { it(`should return true for a span with a valid span type: ${spanType}`, () => { const span = { context: () => ({ _tags: { [SPAN_TYPE]: spanType } }) } - expect(isLLMSpan(span)).to.equal(true) + expect(isLLMObsSpan(span)).to.equal(true) }) } }) From 7b7660d27f80dab292e5bb5faa489172a361c335 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Thu, 3 Oct 2024 16:33:28 -0400 Subject: [PATCH 07/17] warning log for metric --- packages/dd-trace/src/llmobs/tagger.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index 673c5cd53c3..ae95faf18fe 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -87,7 +87,7 @@ class LLMObsTagger { if (typeof value === 'number') { filterdMetrics[key] = value } else { - logger.warn(`Metric "${key}" must be a number, instead got ${value}`) + logger.warn(`Value for metric '${key}' must be a number, instead got ${value}`) } } From a8eeef8ae7791a10b0a34042117cb054e3e514b0 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Fri, 4 Oct 2024 11:29:14 -0400 Subject: [PATCH 08/17] todo-ify --- packages/dd-trace/src/llmobs/span_processor.js | 4 ++++ packages/dd-trace/src/llmobs/tagger.js | 2 ++ 2 files changed, 6 insertions(+) diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js index e6779bbc80f..b6a398bd2c7 100644 --- a/packages/dd-trace/src/llmobs/span_processor.js +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -44,6 +44,8 @@ class LLMObsSpanProcessor { } } + // TODO: can we correlate the span + trace IDs with a namespaced object to + // access LLMObs properties associated with the span? process (span) { if (!this._config.llmobs.enabled) return if (!isLLMObsSpan(span)) return @@ -62,6 +64,8 @@ class LLMObsSpanProcessor { } } + // TODO: pass in span plus correlated LLMObs object from namespaced storage + // Then, the only thing we need the span for are error tags and start/duration format (span) { const tags = span.context()._tags const spanKind = tags[SPAN_KIND] diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index ae95faf18fe..8d8dd9cc0f2 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -30,6 +30,7 @@ class LLMObsTagger { this._config = config } + // TODO: instead of passing in the span here, can we pass in a namespaced object? setLLMObsSpanTags ( span, kind, @@ -57,6 +58,7 @@ class LLMObsTagger { span.setTag(PARENT_ID_KEY, parentId) } + // TODO: similarly for the following `tag` methods, can we pass in a namespaced object instead of the span? tagLLMIO (span, inputData, outputData) { this._tagMessages(span, inputData, INPUT_MESSAGES) this._tagMessages(span, outputData, OUTPUT_MESSAGES) From 80168f80aa4218c51a3bb7789ae49888d69f8627 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Mon, 7 Oct 2024 09:32:54 -0400 Subject: [PATCH 09/17] remove some duplicate logic --- packages/dd-trace/src/llmobs/tagger.js | 104 ++++++++++++------------- 1 file changed, 48 insertions(+), 56 deletions(-) diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index 8d8dd9cc0f2..eb4e233d665 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -145,29 +145,14 @@ class LLMObsTagger { const documentObj = { text } - if (name) { - if (typeof name !== 'string') { - logger.warn('Document name must be a string.') - return undefined - } - documentObj.name = name - } + const validName = this._tagConditionalString(name, 'Document name', documentObj, 'name') + if (!validName) return undefined - if (id) { - if (typeof id !== 'string') { - logger.warn('Document ID must be a string.') - return undefined - } - documentObj.id = id - } + const validId = this._tagConditionalString(id, 'Document ID', documentObj, 'id') + if (!validId) return undefined - if (score) { - if (typeof score !== 'number') { - logger.warn('Document score must be a number.') - return undefined - } - documentObj.score = score - } + const validScore = this._tagConditionalNumber(score, 'Document score', documentObj, 'score') + if (!validScore) return undefined return documentObj }).filter(doc => !!doc) @@ -201,13 +186,8 @@ class LLMObsTagger { return undefined } - if (role) { - if (typeof role !== 'string') { - logger.warn('Message role must be a string.') - return undefined - } - messageObj.role = role - } + const validRole = this._tagConditionalString(role, 'Message role', messageObj, 'role') + if (!validRole) return undefined if (toolCalls) { if (!Array.isArray(toolCalls)) { @@ -223,37 +203,17 @@ class LLMObsTagger { const { name, arguments: args, toolId, type } = toolCall const toolCallObj = {} - if (name) { - if (typeof name !== 'string') { - logger.warn('Tool name must be a string.') - return undefined - } - toolCallObj.name = name - } + const validName = this._tagConditionalString(name, 'Tool name', toolCallObj, 'name') + if (!validName) return undefined - if (args) { - if (typeof args !== 'object') { - logger.warn('Tool arguments must be an object.') - return undefined - } - toolCallObj.arguments = args - } + const validArgs = this._tagConditionalObject(args, 'Tool arguments', toolCallObj, 'arguments') + if (!validArgs) return undefined - if (toolId) { - if (typeof toolId !== 'string') { - logger.warn('Tool ID must be a string.') - return undefined - } - toolCallObj.toolId = toolId - } + const validToolId = this._tagConditionalString(toolId, 'Tool ID', toolCallObj, 'toolId') + if (!validToolId) return undefined - if (type) { - if (typeof type !== 'string') { - logger.warn('Tool type must be a string.') - return undefined - } - toolCallObj.type = type - } + const validType = this._tagConditionalString(type, 'Tool type', toolCallObj, 'type') + if (!validType) return undefined return toolCallObj }).filter(toolCall => !!toolCall) @@ -271,6 +231,38 @@ class LLMObsTagger { } } } + + _tagConditionalString (data, type, carrier, key) { + // returning true here means we won't dropt the whole object (message/document) + // if the field isn't there + if (!data) return true + if (typeof data !== 'string') { + logger.warn(`${type} must be a string.`) + return false + } + carrier[key] = data + return true + } + + _tagConditionalNumber (data, type, carrier, key) { + if (!data) return true + if (typeof data !== 'number') { + logger.warn(`${type} must be a number.`) + return false + } + carrier[key] = data + return true + } + + _tagConditionalObject (data, type, carrier, key) { + if (!data) return true + if (typeof data !== 'object') { + logger.warn(`${type} must be an object.`) + return false + } + carrier[key] = data + return true + } } module.exports = LLMObsTagger From 645577c32c2651627a3b8769e9e1beb9ad0990ab Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Mon, 7 Oct 2024 09:38:41 -0400 Subject: [PATCH 10/17] decouple llmobs span processing with a channel --- packages/dd-trace/src/span_processor.js | 8 ++-- packages/dd-trace/test/span_processor.spec.js | 39 +------------------ 2 files changed, 7 insertions(+), 40 deletions(-) diff --git a/packages/dd-trace/src/span_processor.js b/packages/dd-trace/src/span_processor.js index 870ede6fdfd..4d725943cb8 100644 --- a/packages/dd-trace/src/span_processor.js +++ b/packages/dd-trace/src/span_processor.js @@ -6,11 +6,13 @@ const SpanSampler = require('./span_sampler') const GitMetadataTagger = require('./git_metadata_tagger') const { SpanStatsProcessor } = require('./span_stats') -const LLMObsSpanProcessor = require('./llmobs/span_processor') const startedSpans = new WeakSet() const finishedSpans = new WeakSet() +const { channel } = require('dc-polyfill') +const spanProcessCh = channel('dd-trace:span:process') + class SpanProcessor { constructor (exporter, prioritySampler, config) { this._exporter = exporter @@ -21,7 +23,6 @@ class SpanProcessor { this._stats = new SpanStatsProcessor(config) this._spanSampler = new SpanSampler(config.sampler) this._gitMetadataTagger = new GitMetadataTagger(config) - this._llmobs = new LLMObsSpanProcessor(config) } process (span) { @@ -44,10 +45,11 @@ class SpanProcessor { for (const span of started) { if (span._duration !== undefined) { - this._llmobs.process(span) const formattedSpan = format(span) this._stats.onSpanFinished(formattedSpan) formatted.push(formattedSpan) + + spanProcessCh.publish({ span, formattedSpan }) } else { active.push(span) } diff --git a/packages/dd-trace/test/span_processor.spec.js b/packages/dd-trace/test/span_processor.spec.js index 0bf91cf20d6..5198e1702bc 100644 --- a/packages/dd-trace/test/span_processor.spec.js +++ b/packages/dd-trace/test/span_processor.spec.js @@ -15,8 +15,6 @@ describe('SpanProcessor', () => { let config let SpanSampler let sample - let LLMObsSpanProcessor - let LLMObsSpanProcessorInstance beforeEach(() => { tracer = {} @@ -47,9 +45,6 @@ describe('SpanProcessor', () => { flushMinSpans: 3, stats: { enabled: false - }, - llmobs: { - enabled: false } } format = sinon.stub().returns({ formatted: true }) @@ -59,15 +54,9 @@ describe('SpanProcessor', () => { sample }) - LLMObsSpanProcessorInstance = { - process: sinon.stub() - } - LLMObsSpanProcessor = sinon.stub().returns(LLMObsSpanProcessorInstance) - SpanProcessor = proxyquire('../src/span_processor', { './format': format, - './span_sampler': SpanSampler, - './llmobs/span_processor': LLMObsSpanProcessor + './span_sampler': SpanSampler }) processor = new SpanProcessor(exporter, prioritySampler, config) }) @@ -134,8 +123,7 @@ describe('SpanProcessor', () => { maxPerSecond: 456 } ] - }, - llmobs: { enabled: false } + } } const processor = new SpanProcessor(exporter, prioritySampler, config) @@ -149,9 +137,6 @@ describe('SpanProcessor', () => { tracing: false, stats: { enabled: false - }, - llmobs: { - enabled: false } } @@ -166,24 +151,4 @@ describe('SpanProcessor', () => { expect(finishedSpan.context()).to.have.deep.property('_tags', {}) expect(exporter.export).not.to.have.been.called }) - - it('should initialize and call the LLMObs span processor', () => { - const config = { - llmobs: { - enabled: true - }, - stats: { - enabled: false - } - } - - const processor = new SpanProcessor(exporter, prioritySampler, config) - trace.started = [finishedSpan] - trace.finished = [finishedSpan] - - processor.process(finishedSpan) - - expect(LLMObsSpanProcessor).to.have.been.calledWith(config) - expect(LLMObsSpanProcessorInstance.process).to.have.been.calledWith(finishedSpan) - }) }) From 4a19fec0ffc76a39a9104104beb8ff003f57db0b Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Mon, 7 Oct 2024 10:46:36 -0400 Subject: [PATCH 11/17] use a static weakmap to store llmobs tags/annotations instead of span tags --- packages/dd-trace/src/format.js | 6 +- .../dd-trace/src/llmobs/span_processor.js | 67 +++++----- packages/dd-trace/src/llmobs/tagger.js | 47 ++++--- packages/dd-trace/src/llmobs/util.js | 9 +- packages/dd-trace/test/format.spec.js | 10 -- .../test/llmobs/span_processor.spec.js | 118 ++++++++++-------- packages/dd-trace/test/llmobs/tagger.spec.js | 50 ++++---- packages/dd-trace/test/llmobs/util.spec.js | 27 +--- 8 files changed, 159 insertions(+), 175 deletions(-) diff --git a/packages/dd-trace/src/format.js b/packages/dd-trace/src/format.js index acdf96eaadf..1b7b86d17f0 100644 --- a/packages/dd-trace/src/format.js +++ b/packages/dd-trace/src/format.js @@ -30,8 +30,6 @@ const map = { 'resource.name': 'resource' } -const llmObsTagPrefix = '_ml_obs' - function format (span) { const formatted = formatSpan(span) @@ -162,9 +160,7 @@ function extractTags (trace, span) { break } default: // eslint-disable-line no-fallthrough - if (!tag.startsWith(llmObsTagPrefix)) { // don't add ml_obs-related tags - addTag(trace.meta, trace.metrics, tag, tags[tag]) - } + addTag(trace.meta, trace.metrics, tag, tags[tag]) } } setSingleSpanIngestionTags(trace, context._spanSampling) diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js index b6a398bd2c7..2e8869f1fe8 100644 --- a/packages/dd-trace/src/llmobs/span_processor.js +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -26,9 +26,9 @@ const { ERROR_STACK } = require('../constants') +const LLMObsTagger = require('./tagger') const AgentlessWriter = require('./writers/spans/agentless') const AgentProxyWriter = require('./writers/spans/agentProxy') -const { isLLMObsSpan } = require('./util') const tracerVersion = require('../../../../package.json').version const logger = require('../log') @@ -48,7 +48,8 @@ class LLMObsSpanProcessor { // access LLMObs properties associated with the span? process (span) { if (!this._config.llmobs.enabled) return - if (!isLLMObsSpan(span)) return + // if the span is not in our private tagger map, it is not an llmobs span + if (!LLMObsTagger.tagMap.has(span)) return const formattedEvent = this.format(span) try { @@ -67,56 +68,58 @@ class LLMObsSpanProcessor { // TODO: pass in span plus correlated LLMObs object from namespaced storage // Then, the only thing we need the span for are error tags and start/duration format (span) { - const tags = span.context()._tags - const spanKind = tags[SPAN_KIND] + const spanTags = span.context()._tags + const mlObsTags = LLMObsTagger.tagMap.get(span) + + const spanKind = mlObsTags[SPAN_KIND] const meta = { 'span.kind': spanKind, input: {}, output: {} } const input = {} const output = {} - if (['llm', 'embedding'].includes(spanKind) && tags[MODEL_NAME]) { - meta.model_name = tags[MODEL_NAME] - meta.model_provider = (tags[MODEL_PROVIDER] || 'custom').toLowerCase() + if (['llm', 'embedding'].includes(spanKind) && mlObsTags[MODEL_NAME]) { + meta.model_name = mlObsTags[MODEL_NAME] + meta.model_provider = (mlObsTags[MODEL_PROVIDER] || 'custom').toLowerCase() } - if (tags[METADATA]) { - this._addObject(tags[METADATA], meta.metadata = {}) + if (mlObsTags[METADATA]) { + this._addObject(mlObsTags[METADATA], meta.metadata = {}) } - if (spanKind === 'llm' && tags[INPUT_MESSAGES]) { - input.messages = tags[INPUT_MESSAGES] + if (spanKind === 'llm' && mlObsTags[INPUT_MESSAGES]) { + input.messages = mlObsTags[INPUT_MESSAGES] } - if (tags[INPUT_VALUE]) { - input.value = tags[INPUT_VALUE] + if (mlObsTags[INPUT_VALUE]) { + input.value = mlObsTags[INPUT_VALUE] } - if (spanKind === 'llm' && tags[OUTPUT_MESSAGES]) { - output.messages = tags[OUTPUT_MESSAGES] + if (spanKind === 'llm' && mlObsTags[OUTPUT_MESSAGES]) { + output.messages = mlObsTags[OUTPUT_MESSAGES] } - if (spanKind === 'embedding' && tags[INPUT_DOCUMENTS]) { - input.documents = tags[INPUT_DOCUMENTS] + if (spanKind === 'embedding' && mlObsTags[INPUT_DOCUMENTS]) { + input.documents = mlObsTags[INPUT_DOCUMENTS] } - if (tags[OUTPUT_VALUE]) { - output.value = tags[OUTPUT_VALUE] + if (mlObsTags[OUTPUT_VALUE]) { + output.value = mlObsTags[OUTPUT_VALUE] } - if (spanKind === 'retrieval' && tags[OUTPUT_DOCUMENTS]) { - output.documents = tags[OUTPUT_DOCUMENTS] + if (spanKind === 'retrieval' && mlObsTags[OUTPUT_DOCUMENTS]) { + output.documents = mlObsTags[OUTPUT_DOCUMENTS] } - const error = tags.error + const error = spanTags.error if (error) { - meta[ERROR_MESSAGE] = tags[ERROR_MESSAGE] || error.message || error.code - meta[ERROR_TYPE] = tags[ERROR_TYPE] || error.name - meta[ERROR_STACK] = tags[ERROR_STACK] || error.stack + meta[ERROR_MESSAGE] = spanTags[ERROR_MESSAGE] || error.message || error.code + meta[ERROR_TYPE] = spanTags[ERROR_TYPE] || error.name + meta[ERROR_STACK] = spanTags[ERROR_STACK] || error.stack } if (input) meta.input = input if (output) meta.output = output - const metrics = tags[METRICS] || {} + const metrics = mlObsTags[METRICS] || {} - const mlApp = tags[ML_APP] - const sessionId = tags[SESSION_ID] - const parentId = tags[PARENT_ID_KEY] + const mlApp = mlObsTags[ML_APP] + const sessionId = mlObsTags[SESSION_ID] + const parentId = mlObsTags[PARENT_ID_KEY] - const name = tags[NAME] || span._name + const name = mlObsTags[NAME] || span._name const llmObsSpanEvent = { trace_id: span.context().toTraceId(true), @@ -126,7 +129,7 @@ class LLMObsSpanProcessor { tags: this._processTags(span, mlApp, sessionId, error), start_ns: Math.round(span._startTime * 1e6), duration: Math.round(span._duration * 1e6), - status: tags.error ? 'error' : 'ok', + status: spanTags.error ? 'error' : 'ok', meta, metrics, _dd: { @@ -190,7 +193,7 @@ class LLMObsSpanProcessor { const errType = span.context()._tags[ERROR_TYPE] || error?.name if (errType) tags.error_type = errType if (sessionId) tags.session_id = sessionId - const existingTags = span.context()._tags[TAGS] || {} + const existingTags = LLMObsTagger.tagMap.get(span)?.[TAGS] || {} if (existingTags) tags = { ...tags, ...existingTags } return Object.entries(tags).map(([key, value]) => `${key}:${value ?? ''}`) } diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index eb4e233d665..6b639d697c4 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -25,11 +25,24 @@ const { ROOT_PARENT_ID } = require('./constants') +// maps spans to tag annotations +const tagMap = new WeakMap() + +function setTag (span, key, value) { + const tagsCarrier = tagMap.get(span) || {} + Object.assign(tagsCarrier, { [key]: value }) + if (!tagMap.has(span)) tagMap.set(span, tagsCarrier) +} + class LLMObsTagger { constructor (config) { this._config = config } + static get tagMap () { + return tagMap + } + // TODO: instead of passing in the span here, can we pass in a namespaced object? setLLMObsSpanTags ( span, @@ -38,24 +51,24 @@ class LLMObsTagger { name ) { if (!this._config.llmobs.enabled) return - if (kind) span.setTag(SPAN_TYPE, 'llm') // only mark it as an llm span if it was a valid kind - if (name) span.setTag(NAME, name) + if (kind) setTag(span, SPAN_TYPE, 'llm') // only mark it as an llm span if it was a valid kind + if (name) setTag(span, NAME, name) - span.setTag(SPAN_KIND, kind) - if (modelName) span.setTag(MODEL_NAME, modelName) - if (modelProvider) span.setTag(MODEL_PROVIDER, modelProvider) + setTag(span, SPAN_KIND, kind) + if (modelName) setTag(span, MODEL_NAME, modelName) + if (modelProvider) setTag(span, MODEL_PROVIDER, modelProvider) sessionId = sessionId || parentLLMObsSpan?.context()._tags[SESSION_ID] - if (sessionId) span.setTag(SESSION_ID, sessionId) + if (sessionId) setTag(span, SESSION_ID, sessionId) if (!mlApp) mlApp = parentLLMObsSpan?.context()._tags[ML_APP] || this._config.llmobs.mlApp - span.setTag(ML_APP, mlApp) + setTag(span, ML_APP, mlApp) const parentId = parentLLMObsSpan?.context().toSpanId() || span.context()._trace.tags[PROPAGATED_PARENT_ID_KEY] || ROOT_PARENT_ID - span.setTag(PARENT_ID_KEY, parentId) + setTag(span, PARENT_ID_KEY, parentId) } // TODO: similarly for the following `tag` methods, can we pass in a namespaced object instead of the span? @@ -80,7 +93,7 @@ class LLMObsTagger { } tagMetadata (span, metadata) { - span.setTag(METADATA, metadata) + setTag(span, METADATA, metadata) } tagMetrics (span, metrics) { @@ -93,25 +106,25 @@ class LLMObsTagger { } } - span.setTag(METRICS, filterdMetrics) + setTag(span, METRICS, filterdMetrics) } tagSpanTags (span, tags) { // new tags will be merged with existing tags - const currentTags = span.context()._tags[TAGS] + const currentTags = tagMap.get(span)?.[TAGS] if (currentTags) { Object.assign(tags, currentTags) } - span.setTag(TAGS, tags) + setTag(span, TAGS, tags) } _tagText (span, data, key) { if (data) { if (typeof data === 'string') { - span.setTag(key, data) + setTag(span, key, data) } else { try { - span.setTag(key, JSON.stringify(data)) + setTag(span, key, JSON.stringify(data)) } catch { const type = key === INPUT_VALUE ? 'input' : 'output' logger.warn(`Failed to parse ${type} value, must be JSON serializable.`) @@ -157,7 +170,7 @@ class LLMObsTagger { return documentObj }).filter(doc => !!doc) - span.setTag(key, documents) + setTag(span, key, documents) } } @@ -227,13 +240,13 @@ class LLMObsTagger { }).filter(msg => !!msg) if (messages.length) { - span.setTag(key, messages) + setTag(span, key, messages) } } } _tagConditionalString (data, type, carrier, key) { - // returning true here means we won't dropt the whole object (message/document) + // returning true here means we won't drop the whole object (message/document) // if the field isn't there if (!data) return true if (typeof data !== 'string') { diff --git a/packages/dd-trace/src/llmobs/util.js b/packages/dd-trace/src/llmobs/util.js index ae31b6f7adb..5159a72b3b8 100644 --- a/packages/dd-trace/src/llmobs/util.js +++ b/packages/dd-trace/src/llmobs/util.js @@ -1,7 +1,5 @@ 'use strict' -const { SPAN_TYPE } = require('../../../../ext/tags') - function encodeUnicode (str) { if (!str) return str return str.split('').map(char => { @@ -13,11 +11,6 @@ function encodeUnicode (str) { }).join('') } -function isLLMObsSpan (span) { - return ['llm', 'openai'].includes(span?.context()._tags[SPAN_TYPE]) -} - module.exports = { - encodeUnicode, - isLLMObsSpan + encodeUnicode } diff --git a/packages/dd-trace/test/format.spec.js b/packages/dd-trace/test/format.spec.js index b103ea323a6..846a02cd66a 100644 --- a/packages/dd-trace/test/format.spec.js +++ b/packages/dd-trace/test/format.spec.js @@ -577,15 +577,5 @@ describe('format', () => { expect(trace.metrics).to.have.property('_dd1.sr.eausr', 1) }) - - it('should skip formatting temporary _ml_obs.* tags', () => { - spanContext._tags['_ml_obs.meta.span.kind'] = 'llm' - spanContext._tags['_ml_obs.meta.ml_app'] = 'test' - - trace = format(span) - - expect(trace.meta['_ml_obs.meta.span.kind']).to.be.undefined - expect(trace.meta['_ml_obs.meta.ml_app']).to.be.undefined - }) }) }) diff --git a/packages/dd-trace/test/llmobs/span_processor.spec.js b/packages/dd-trace/test/llmobs/span_processor.spec.js index e6388ec2fe2..d98f3114700 100644 --- a/packages/dd-trace/test/llmobs/span_processor.spec.js +++ b/packages/dd-trace/test/llmobs/span_processor.spec.js @@ -3,6 +3,9 @@ const { expect } = require('chai') const proxyquire = require('proxyquire') +// we will use this to populate the span-tags map +const LLMObsTagger = require('../../src/llmobs/tagger') + describe('span processor', () => { let LLMObsSpanProcessor let processor @@ -72,24 +75,24 @@ describe('span processor', () => { _duration: 1, // this is in ms, will be converted to ns context () { return { - _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'llm', - '_ml_obs.meta.model_name': 'myModel', - '_ml_obs.meta.model_provider': 'myProvider', - '_ml_obs.meta.metadata': { foo: 'bar' }, - '_ml_obs.meta.ml_app': 'myApp', - '_ml_obs.meta.input.value': 'input-value', - '_ml_obs.meta.output.value': 'output-value', - '_ml_obs.meta.input.messages': [{ role: 'user', content: 'hello' }], - '_ml_obs.meta.output.messages': [{ role: 'assistant', content: 'world' }], - '_ml_obs.llmobs_parent_id': '1234' - }, + _tags: {}, toTraceId () { return '123' }, // should not use this toSpanId () { return '456' } } } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.meta.model_name': 'myModel', + '_ml_obs.meta.model_provider': 'myProvider', + '_ml_obs.meta.metadata': { foo: 'bar' }, + '_ml_obs.meta.ml_app': 'myApp', + '_ml_obs.meta.input.value': 'input-value', + '_ml_obs.meta.output.value': 'output-value', + '_ml_obs.meta.input.messages': [{ role: 'user', content: 'hello' }], + '_ml_obs.meta.output.messages': [{ role: 'assistant', content: 'world' }], + '_ml_obs.llmobs_parent_id': '1234' + }) processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) @@ -152,17 +155,18 @@ describe('span processor', () => { span = { context () { return { - _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'llm', - '_ml_obs.meta.metadata': metadata - }, + _tags: {}, toTraceId () { return '123' }, toSpanId () { return '456' } } } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.meta.metadata': metadata + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) const payload = writer.append.getCall(0).firstArg @@ -179,17 +183,18 @@ describe('span processor', () => { span = { context () { return { - _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'retrieval', - '_ml_obs.meta.output.documents': [{ text: 'hello', name: 'myDoc', id: '1', score: 0.6 }] - }, + _tags: {}, toTraceId () { return '123' }, toSpanId () { return '456' } } } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'retrieval', + '_ml_obs.meta.output.documents': [{ text: 'hello', name: 'myDoc', id: '1', score: 0.6 }] + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) @@ -207,17 +212,18 @@ describe('span processor', () => { span = { context () { return { - _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'embedding', - '_ml_obs.meta.input.documents': [{ text: 'hello', name: 'myDoc', id: '1', score: 0.6 }] - }, + _tags: {}, toTraceId () { return '123' }, toSpanId () { return '456' } } } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'embedding', + '_ml_obs.meta.input.documents': [{ text: 'hello', name: 'myDoc', id: '1', score: 0.6 }] + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) @@ -235,17 +241,18 @@ describe('span processor', () => { span = { context () { return { - _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'llm', - '_ml_obs.meta.model_name': 'myModel' - }, + _tags: {}, toTraceId () { return '123' }, toSpanId () { return '456' } } } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.meta.model_name': 'myModel' + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) @@ -259,8 +266,6 @@ describe('span processor', () => { context () { return { _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'llm', error: new Error(), 'error.message': 'error message', 'error.type': 'error type', @@ -272,6 +277,10 @@ describe('span processor', () => { } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'llm' + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) @@ -290,8 +299,6 @@ describe('span processor', () => { context () { return { _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'llm', error: new Error('error message') }, toTraceId () { return '123' }, @@ -300,6 +307,10 @@ describe('span processor', () => { } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'llm' + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) @@ -318,17 +329,18 @@ describe('span processor', () => { _name: 'test', context () { return { - _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'llm', - '_ml_obs.name': 'mySpan' - }, + _tags: {}, toTraceId () { return '123' }, toSpanId () { return '456' } } } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.name': 'mySpan' + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) @@ -341,17 +353,18 @@ describe('span processor', () => { span = { context () { return { - _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'llm', - '_ml_obs.session_id': '1234' - }, + _tags: {}, toTraceId () { return '123' }, toSpanId () { return '456' } } } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.session_id': '1234' + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) @@ -365,17 +378,18 @@ describe('span processor', () => { span = { context () { return { - _tags: { - 'span.type': 'llm', - '_ml_obs.meta.span.kind': 'llm', - '_ml_obs.tags': { hostname: 'localhost', foo: 'bar', source: 'mySource' } - }, + _tags: {}, toTraceId () { return '123' }, toSpanId () { return '456' } } } } + LLMObsTagger.tagMap.set(span, { + '_ml_obs.meta.span.kind': 'llm', + '_ml_obs.tags': { hostname: 'localhost', foo: 'bar', source: 'mySource' } + }) + processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) processor.process(span) diff --git a/packages/dd-trace/test/llmobs/tagger.spec.js b/packages/dd-trace/test/llmobs/tagger.spec.js index 35768ffbb7b..2dac8c1a2d4 100644 --- a/packages/dd-trace/test/llmobs/tagger.spec.js +++ b/packages/dd-trace/test/llmobs/tagger.spec.js @@ -51,13 +51,13 @@ describe('tagger', () => { tagger = new Tagger({ llmobs: { enabled: false } }) tagger.setLLMObsSpanTags(span, 'llm') - expect(span.context()._tags).to.deep.equal({}) + expect(Tagger.tagMap.get(span)).to.deep.equal(undefined) }) it('tags an llm obs span with basic and default properties', () => { tagger.setLLMObsSpanTags(span, 'workflow') - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ 'span.type': 'llm', '_ml_obs.meta.span.kind': 'workflow', '_ml_obs.meta.ml_app': 'my-default-ml-app', @@ -73,7 +73,7 @@ describe('tagger', () => { mlApp: 'my-app' }) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.model_name': 'my-model', @@ -87,7 +87,7 @@ describe('tagger', () => { it('uses the name if provided', () => { tagger.setLLMObsSpanTags(span, 'llm', {}, 'my-span-name') - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-default-ml-app', @@ -99,7 +99,7 @@ describe('tagger', () => { it('defaults parent id to undefined', () => { tagger.setLLMObsSpanTags(span, 'llm') - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-default-ml-app', @@ -121,7 +121,7 @@ describe('tagger', () => { } tagger.setLLMObsSpanTags(span, 'llm', { parentLLMObsSpan: parentSpan }) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-ml-app', @@ -133,7 +133,7 @@ describe('tagger', () => { it('uses the propagated trace id if provided', () => { tagger.setLLMObsSpanTags(span, 'llm') - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-default-ml-app', @@ -146,7 +146,7 @@ describe('tagger', () => { tagger.setLLMObsSpanTags(span, 'llm') - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-default-ml-app', @@ -157,14 +157,14 @@ describe('tagger', () => { it('does not set span type if the LLMObs span kind is falsy', () => { tagger.setLLMObsSpanTags(span, false) - expect(span.context()._tags['span.type']).to.be.undefined + expect(Tagger.tagMap.get(span)['span.type']).to.be.undefined }) }) describe('tagMetadata', () => { it('tags a span with metadata', () => { tagger.tagMetadata(span, { a: 'foo', b: 'bar' }) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.metadata': { a: 'foo', b: 'bar' } }) }) @@ -173,7 +173,7 @@ describe('tagger', () => { describe('tagMetrics', () => { it('tags a span with metrics', () => { tagger.tagMetadata(span, { a: 1, b: 2 }) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.metadata': { a: 1, b: 2 } }) }) @@ -186,7 +186,7 @@ describe('tagger', () => { d: undefined } tagger.tagMetrics(span, metrics) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.metrics': { a: 1 } }) @@ -198,16 +198,16 @@ describe('tagger', () => { it('sets tags on a span', () => { const tags = { foo: 'bar' } tagger.tagSpanTags(span, tags) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.tags': { foo: 'bar' } }) }) it('merges tags so they do not overwrite', () => { - span.context()._tags['_ml_obs.tags'] = { a: 1 } + Tagger.tagMap.set(span, { '_ml_obs.tags': { a: 1 } }) const tags = { a: 2, b: 1 } tagger.tagSpanTags(span, tags) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.tags': { a: 1, b: 1 } }) }) @@ -226,7 +226,7 @@ describe('tagger', () => { const outputData = 'Nice to meet you, human!' tagger.tagLLMIO(span, inputData, outputData) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.messages': [ { content: 'you are an amazing assistant' }, { content: 'hello! my name is foobar' }, @@ -252,7 +252,7 @@ describe('tagger', () => { { content: 'goodbye', role: 5 } ] tagger.tagLLMIO(span, inputData, outputData) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.messages': [{ content: 'hi' }] }) @@ -276,7 +276,7 @@ describe('tagger', () => { ] tagger.tagLLMIO(span, inputData, outputData) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.messages': [ { content: 'hello', @@ -306,7 +306,7 @@ describe('tagger', () => { ] tagger.tagLLMIO(span, inputData, undefined) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.messages': [ { content: 'a' }, { content: 'b' }, @@ -340,7 +340,7 @@ describe('tagger', () => { ] const outputData = 'embedded documents' tagger.tagEmbeddingIO(span, inputData, outputData) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.documents': [ { text: 'my string document' }, { text: 'my object document' }, @@ -363,7 +363,7 @@ describe('tagger', () => { ] const outputData = 'output' tagger.tagEmbeddingIO(span, inputData, outputData) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.documents': [{ text: 'hi' }], '_ml_obs.meta.output.value': 'output' }) @@ -389,7 +389,7 @@ describe('tagger', () => { ] tagger.tagRetrievalIO(span, inputData, outputData) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.value': 'some query', '_ml_obs.meta.output.documents': [ { text: 'result 1' }, @@ -412,7 +412,7 @@ describe('tagger', () => { undefined ] tagger.tagRetrievalIO(span, inputData, outputData) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.value': 'some query', '_ml_obs.meta.output.documents': [{ text: 'hi' }] }) @@ -430,7 +430,7 @@ describe('tagger', () => { const inputData = { some: 'object' } const outputData = 'some text' tagger.tagTextIO(span, inputData, outputData) - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.input.value': '{"some":"object"}', '_ml_obs.meta.output.value': 'some text' }) @@ -440,7 +440,7 @@ describe('tagger', () => { const data = unserializbleObject() tagger.tagTextIO(span, data, 'output') expect(logger.warn).to.have.been.calledOnceWith('Failed to parse input value, must be JSON serializable.') - expect(span.context()._tags).to.deep.equal({ + expect(Tagger.tagMap.get(span)).to.deep.equal({ '_ml_obs.meta.output.value': 'output' }) }) diff --git a/packages/dd-trace/test/llmobs/util.spec.js b/packages/dd-trace/test/llmobs/util.spec.js index 987004dc8b8..e365c01e13b 100644 --- a/packages/dd-trace/test/llmobs/util.spec.js +++ b/packages/dd-trace/test/llmobs/util.spec.js @@ -1,9 +1,7 @@ 'use strict' -const { SPAN_TYPE } = require('../../../../ext/tags') const { - encodeUnicode, - isLLMObsSpan + encodeUnicode } = require('../../src/llmobs/util') describe('util', () => { @@ -16,27 +14,4 @@ describe('util', () => { expect(encodeUnicode('test 😀')).to.equal('test \\ud83d\\ude00') }) }) - - describe('isLLMSpan', () => { - it('should return false for an undefined span', () => { - expect(isLLMObsSpan(undefined)).to.equal(false) - }) - - it('should return false for a span without a SPAN_KIND tag', () => { - const span = { context: () => ({ _tags: {} }) } - expect(isLLMObsSpan(span)).to.equal(false) - }) - - it('should return false for a span with an invalid span type', () => { - const span = { context: () => ({ _tags: { [SPAN_TYPE]: 'invalid' } }) } - expect(isLLMObsSpan(span)).to.equal(false) - }) - - for (const spanType of ['llm', 'openai']) { - it(`should return true for a span with a valid span type: ${spanType}`, () => { - const span = { context: () => ({ _tags: { [SPAN_TYPE]: spanType } }) } - expect(isLLMObsSpan(span)).to.equal(true) - }) - } - }) }) From 070b564c8dac4d211c1fc16c6e982b36b724ef91 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Mon, 7 Oct 2024 14:28:13 -0400 Subject: [PATCH 12/17] do not register span in map if it does not have an llmobs span kind --- packages/dd-trace/src/llmobs/tagger.js | 7 ++----- packages/dd-trace/test/llmobs/tagger.spec.js | 9 +-------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index 6b639d697c4..c9d97bd7ee2 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -1,9 +1,6 @@ 'use strict' const logger = require('../log') -const { - SPAN_TYPE -} = require('../../../../ext/tags') const { MODEL_NAME, MODEL_PROVIDER, @@ -51,7 +48,7 @@ class LLMObsTagger { name ) { if (!this._config.llmobs.enabled) return - if (kind) setTag(span, SPAN_TYPE, 'llm') // only mark it as an llm span if it was a valid kind + if (!kind) return // do not register it in the map if it doesn't have an llmobs span kind if (name) setTag(span, NAME, name) setTag(span, SPAN_KIND, kind) @@ -222,7 +219,7 @@ class LLMObsTagger { const validArgs = this._tagConditionalObject(args, 'Tool arguments', toolCallObj, 'arguments') if (!validArgs) return undefined - const validToolId = this._tagConditionalString(toolId, 'Tool ID', toolCallObj, 'toolId') + const validToolId = this._tagConditionalString(toolId, 'Tool ID', toolCallObj, 'tool_id') if (!validToolId) return undefined const validType = this._tagConditionalString(type, 'Tool type', toolCallObj, 'type') diff --git a/packages/dd-trace/test/llmobs/tagger.spec.js b/packages/dd-trace/test/llmobs/tagger.spec.js index 2dac8c1a2d4..cc618d7167d 100644 --- a/packages/dd-trace/test/llmobs/tagger.spec.js +++ b/packages/dd-trace/test/llmobs/tagger.spec.js @@ -58,7 +58,6 @@ describe('tagger', () => { tagger.setLLMObsSpanTags(span, 'workflow') expect(Tagger.tagMap.get(span)).to.deep.equal({ - 'span.type': 'llm', '_ml_obs.meta.span.kind': 'workflow', '_ml_obs.meta.ml_app': 'my-default-ml-app', '_ml_obs.llmobs_parent_id': 'undefined' // no parent id provided @@ -74,7 +73,6 @@ describe('tagger', () => { }) expect(Tagger.tagMap.get(span)).to.deep.equal({ - 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.model_name': 'my-model', '_ml_obs.meta.model_provider': 'my-provider', @@ -88,7 +86,6 @@ describe('tagger', () => { tagger.setLLMObsSpanTags(span, 'llm', {}, 'my-span-name') expect(Tagger.tagMap.get(span)).to.deep.equal({ - 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-default-ml-app', '_ml_obs.llmobs_parent_id': 'undefined', @@ -100,7 +97,6 @@ describe('tagger', () => { tagger.setLLMObsSpanTags(span, 'llm') expect(Tagger.tagMap.get(span)).to.deep.equal({ - 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-default-ml-app', '_ml_obs.llmobs_parent_id': 'undefined' @@ -122,7 +118,6 @@ describe('tagger', () => { tagger.setLLMObsSpanTags(span, 'llm', { parentLLMObsSpan: parentSpan }) expect(Tagger.tagMap.get(span)).to.deep.equal({ - 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-ml-app', '_ml_obs.session_id': 'my-session', @@ -134,7 +129,6 @@ describe('tagger', () => { tagger.setLLMObsSpanTags(span, 'llm') expect(Tagger.tagMap.get(span)).to.deep.equal({ - 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-default-ml-app', '_ml_obs.llmobs_parent_id': 'undefined' @@ -147,7 +141,6 @@ describe('tagger', () => { tagger.setLLMObsSpanTags(span, 'llm') expect(Tagger.tagMap.get(span)).to.deep.equal({ - 'span.type': 'llm', '_ml_obs.meta.span.kind': 'llm', '_ml_obs.meta.ml_app': 'my-default-ml-app', '_ml_obs.llmobs_parent_id': '-567' @@ -157,7 +150,7 @@ describe('tagger', () => { it('does not set span type if the LLMObs span kind is falsy', () => { tagger.setLLMObsSpanTags(span, false) - expect(Tagger.tagMap.get(span)['span.type']).to.be.undefined + expect(Tagger.tagMap.get(span)).to.be.undefined }) }) From 3b12cb519b214e49d61e7a273c4ddcddd64500cf Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Mon, 7 Oct 2024 14:28:41 -0400 Subject: [PATCH 13/17] span is passed on an object from sp publisher --- .../dd-trace/src/llmobs/span_processor.js | 2 +- .../test/llmobs/span_processor.spec.js | 22 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js index 2e8869f1fe8..ae78a105183 100644 --- a/packages/dd-trace/src/llmobs/span_processor.js +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -46,7 +46,7 @@ class LLMObsSpanProcessor { // TODO: can we correlate the span + trace IDs with a namespaced object to // access LLMObs properties associated with the span? - process (span) { + process ({ span }) { if (!this._config.llmobs.enabled) return // if the span is not in our private tagger map, it is not an llmobs span if (!LLMObsTagger.tagMap.has(span)) return diff --git a/packages/dd-trace/test/llmobs/span_processor.spec.js b/packages/dd-trace/test/llmobs/span_processor.spec.js index d98f3114700..a824eae2f5e 100644 --- a/packages/dd-trace/test/llmobs/span_processor.spec.js +++ b/packages/dd-trace/test/llmobs/span_processor.spec.js @@ -58,7 +58,7 @@ describe('span processor', () => { it('should do nothing if llmobs is not enabled', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: false } }) - expect(() => processor.process(span)).not.to.throw() + expect(() => processor.process({ span })).not.to.throw() }) it('should do nothing if the span is not an llm obs span', () => { @@ -96,7 +96,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload).to.deep.equal({ @@ -168,7 +168,7 @@ describe('span processor', () => { }) processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.meta.metadata).to.deep.equal({ @@ -197,7 +197,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.meta.output.documents).to.deep.equal([{ @@ -226,7 +226,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.meta.input.documents).to.deep.equal([{ @@ -255,7 +255,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.meta.model_provider).to.equal('custom') @@ -283,7 +283,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.meta['error.message']).to.equal('error message') @@ -313,7 +313,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.meta['error.message']).to.equal('error message') @@ -343,7 +343,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.name).to.equal('mySpan') @@ -367,7 +367,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.session_id).to.equal('1234') @@ -392,7 +392,7 @@ describe('span processor', () => { processor = new LLMObsSpanProcessor({ llmobs: { enabled: true } }) - processor.process(span) + processor.process({ span }) const payload = writer.append.getCall(0).firstArg expect(payload.tags).to.include('foo:bar') From 8fc59adcf26843becc373029dfa71c5db15728e5 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Mon, 7 Oct 2024 14:31:56 -0400 Subject: [PATCH 14/17] re-clarify TODOs --- packages/dd-trace/src/llmobs/span_processor.js | 5 +---- packages/dd-trace/src/llmobs/tagger.js | 5 +++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js index ae78a105183..6afc389020f 100644 --- a/packages/dd-trace/src/llmobs/span_processor.js +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -44,8 +44,7 @@ class LLMObsSpanProcessor { } } - // TODO: can we correlate the span + trace IDs with a namespaced object to - // access LLMObs properties associated with the span? + // TODO: instead of relying on the tagger's weakmap registry, can we use some namespaced storage correlation? process ({ span }) { if (!this._config.llmobs.enabled) return // if the span is not in our private tagger map, it is not an llmobs span @@ -65,8 +64,6 @@ class LLMObsSpanProcessor { } } - // TODO: pass in span plus correlated LLMObs object from namespaced storage - // Then, the only thing we need the span for are error tags and start/duration format (span) { const spanTags = span.context()._tags const mlObsTags = LLMObsTagger.tagMap.get(span) diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index c9d97bd7ee2..844e52bbf10 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -40,7 +40,7 @@ class LLMObsTagger { return tagMap } - // TODO: instead of passing in the span here, can we pass in a namespaced object? + // TODO: we're using a weakmap registry of LLMObs spans for now, how can this be used in the core API? setLLMObsSpanTags ( span, kind, @@ -68,7 +68,8 @@ class LLMObsTagger { setTag(span, PARENT_ID_KEY, parentId) } - // TODO: similarly for the following `tag` methods, can we pass in a namespaced object instead of the span? + // TODO: similarly for the following `tag` methods, + // how can we transition from a span weakmap to core API functionality tagLLMIO (span, inputData, outputData) { this._tagMessages(span, inputData, INPUT_MESSAGES) this._tagMessages(span, outputData, OUTPUT_MESSAGES) From 2698b5fb1c7b4a87539dbb84c5142e289ad884a6 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Mon, 7 Oct 2024 15:29:54 -0400 Subject: [PATCH 15/17] only send span in publish --- packages/dd-trace/src/span_processor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/span_processor.js b/packages/dd-trace/src/span_processor.js index 4d725943cb8..deb92c02f34 100644 --- a/packages/dd-trace/src/span_processor.js +++ b/packages/dd-trace/src/span_processor.js @@ -49,7 +49,7 @@ class SpanProcessor { this._stats.onSpanFinished(formattedSpan) formatted.push(formattedSpan) - spanProcessCh.publish({ span, formattedSpan }) + spanProcessCh.publish({ span }) } else { active.push(span) } From bdf5fc0c51412199bdfca72a2f11f953da4e5b5d Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Mon, 7 Oct 2024 15:30:37 -0400 Subject: [PATCH 16/17] log multiple warnings and return conditional undefined --- packages/dd-trace/src/llmobs/tagger.js | 57 +++++++++----------- packages/dd-trace/test/llmobs/tagger.spec.js | 28 ++++++++++ 2 files changed, 54 insertions(+), 31 deletions(-) diff --git a/packages/dd-trace/src/llmobs/tagger.js b/packages/dd-trace/src/llmobs/tagger.js index 844e52bbf10..0b5a8aa778b 100644 --- a/packages/dd-trace/src/llmobs/tagger.js +++ b/packages/dd-trace/src/llmobs/tagger.js @@ -142,33 +142,32 @@ class LLMObsTagger { return { text: document } } + let validDocument = true + if (document == null || typeof document !== 'object') { logger.warn('Documents must be a string, object, or list of objects.') - return undefined + return undefined // returning here as we need document to be an object } const { text, name, id, score } = document if (typeof text !== 'string') { logger.warn('Document text must be a string.') - return undefined + validDocument = false } const documentObj = { text } - const validName = this._tagConditionalString(name, 'Document name', documentObj, 'name') - if (!validName) return undefined - - const validId = this._tagConditionalString(id, 'Document ID', documentObj, 'id') - if (!validId) return undefined - - const validScore = this._tagConditionalNumber(score, 'Document score', documentObj, 'score') - if (!validScore) return undefined + validDocument = this._tagConditionalString(name, 'Document name', documentObj, 'name') && validDocument + validDocument = this._tagConditionalString(id, 'Document ID', documentObj, 'id') && validDocument + validDocument = this._tagConditionalNumber(score, 'Document score', documentObj, 'score') && validDocument - return documentObj + return validDocument ? documentObj : undefined }).filter(doc => !!doc) - setTag(span, key, documents) + if (documents.length) { + setTag(span, key, documents) + } } } @@ -185,20 +184,21 @@ class LLMObsTagger { if (message == null || typeof message !== 'object') { logger.warn('Messages must be a string, object, or list of objects') - return undefined + return undefined // returning here as we need message to be an object } + let validMessage = true + const { content = '', role } = message let toolCalls = message.toolCalls const messageObj = { content } if (typeof content !== 'string') { logger.warn('Message content must be a string.') - return undefined + validMessage = false } - const validRole = this._tagConditionalString(role, 'Message role', messageObj, 'role') - if (!validRole) return undefined + validMessage = this._tagConditionalString(role, 'Message role', messageObj, 'role') && validMessage if (toolCalls) { if (!Array.isArray(toolCalls)) { @@ -208,25 +208,20 @@ class LLMObsTagger { const filteredToolCalls = toolCalls.map(toolCall => { if (typeof toolCall !== 'object') { logger.warn('Tool call must be an object.') - return undefined + return undefined // returning here as we need tool call to be an object } + let validTool = true + const { name, arguments: args, toolId, type } = toolCall const toolCallObj = {} - const validName = this._tagConditionalString(name, 'Tool name', toolCallObj, 'name') - if (!validName) return undefined - - const validArgs = this._tagConditionalObject(args, 'Tool arguments', toolCallObj, 'arguments') - if (!validArgs) return undefined - - const validToolId = this._tagConditionalString(toolId, 'Tool ID', toolCallObj, 'tool_id') - if (!validToolId) return undefined - - const validType = this._tagConditionalString(type, 'Tool type', toolCallObj, 'type') - if (!validType) return undefined + validTool = this._tagConditionalString(name, 'Tool name', toolCallObj, 'name') && validTool + validTool = this._tagConditionalObject(args, 'Tool arguments', toolCallObj, 'arguments') && validTool + validTool = this._tagConditionalString(toolId, 'Tool ID', toolCallObj, 'tool_id') && validTool + validTool = this._tagConditionalString(type, 'Tool type', toolCallObj, 'type') && validTool - return toolCallObj + return validTool ? toolCallObj : undefined }).filter(toolCall => !!toolCall) if (filteredToolCalls.length) { @@ -234,7 +229,7 @@ class LLMObsTagger { } } - return messageObj + return validMessage ? messageObj : undefined }).filter(msg => !!msg) if (messages.length) { @@ -245,7 +240,7 @@ class LLMObsTagger { _tagConditionalString (data, type, carrier, key) { // returning true here means we won't drop the whole object (message/document) - // if the field isn't there + // if the field isn't there. we check for mandatory fields separately if (!data) return true if (typeof data !== 'string') { logger.warn(`${type} must be a string.`) diff --git a/packages/dd-trace/test/llmobs/tagger.spec.js b/packages/dd-trace/test/llmobs/tagger.spec.js index cc618d7167d..dd21a53f9ee 100644 --- a/packages/dd-trace/test/llmobs/tagger.spec.js +++ b/packages/dd-trace/test/llmobs/tagger.spec.js @@ -318,6 +318,20 @@ describe('tagger', () => { expect(logger.warn.getCall(5).firstArg).to.equal('Tool type must be a string.') expect(logger.warn.getCall(6).firstArg).to.equal('Tool arguments must be an object.') }) + + it('logs multiple errors if there are multiple errors for a message and filters it out', () => { + const messages = [ + { content: 'a', toolCalls: [5, { name: 5, type: 7 }], role: 7 } + ] + + tagger.tagLLMIO(span, messages, undefined) + expect(Tagger.tagMap.get(span)).to.deep.equal(undefined) + + expect(logger.warn.getCall(0).firstArg).to.equal('Message role must be a string.') + expect(logger.warn.getCall(1).firstArg).to.equal('Tool call must be an object.') + expect(logger.warn.getCall(2).firstArg).to.equal('Tool name must be a string.') + expect(logger.warn.getCall(3).firstArg).to.equal('Tool type must be a string.') + }) }) }) @@ -367,6 +381,20 @@ describe('tagger', () => { expect(logger.warn.getCall(3).firstArg).to.equal('Documents must be a string, object, or list of objects.') expect(logger.warn.getCall(4).firstArg).to.equal('Documents must be a string, object, or list of objects.') }) + + it('logs multiple errors if there are multiple errors for a document and filters it out', () => { + const documents = [ + { text: 'a', name: 5, id: 7, score: 9 } + ] + + tagger.tagEmbeddingIO(span, documents, 'output') + expect(Tagger.tagMap.get(span)).to.deep.equal({ + '_ml_obs.meta.output.value': 'output' + }) + + expect(logger.warn.getCall(0).firstArg).to.equal('Document name must be a string.') + expect(logger.warn.getCall(1).firstArg).to.equal('Document ID must be a string.') + }) }) describe('tagRetrievalIO', () => { From c2408d0f106e0e4c9e33b0a501712eb2f42f84e5 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Tue, 8 Oct 2024 16:33:51 -0400 Subject: [PATCH 17/17] update error logic --- packages/dd-trace/src/llmobs/span_processor.js | 4 ++-- packages/dd-trace/test/llmobs/span_processor.spec.js | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/dd-trace/src/llmobs/span_processor.js b/packages/dd-trace/src/llmobs/span_processor.js index 6afc389020f..c87e798eb00 100644 --- a/packages/dd-trace/src/llmobs/span_processor.js +++ b/packages/dd-trace/src/llmobs/span_processor.js @@ -100,7 +100,7 @@ class LLMObsSpanProcessor { output.documents = mlObsTags[OUTPUT_DOCUMENTS] } - const error = spanTags.error + const error = spanTags.error || spanTags[ERROR_TYPE] if (error) { meta[ERROR_MESSAGE] = spanTags[ERROR_MESSAGE] || error.message || error.code meta[ERROR_TYPE] = spanTags[ERROR_TYPE] || error.name @@ -126,7 +126,7 @@ class LLMObsSpanProcessor { tags: this._processTags(span, mlApp, sessionId, error), start_ns: Math.round(span._startTime * 1e6), duration: Math.round(span._duration * 1e6), - status: spanTags.error ? 'error' : 'ok', + status: error ? 'error' : 'ok', meta, metrics, _dd: { diff --git a/packages/dd-trace/test/llmobs/span_processor.spec.js b/packages/dd-trace/test/llmobs/span_processor.spec.js index a824eae2f5e..8265d44cdb0 100644 --- a/packages/dd-trace/test/llmobs/span_processor.spec.js +++ b/packages/dd-trace/test/llmobs/span_processor.spec.js @@ -266,7 +266,6 @@ describe('span processor', () => { context () { return { _tags: { - error: new Error(), 'error.message': 'error message', 'error.type': 'error type', 'error.stack': 'error stack'