From 4bc99e2c35e6128a70be6374319ca56f39640f64 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Tue, 17 Sep 2024 15:04:54 -0400 Subject: [PATCH 1/6] config changes --- packages/dd-trace/src/config.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 0060062b4eb..e17a0dccf10 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -284,6 +284,11 @@ class Config { options.appsec = {} } + const DD_LLMOBS_ENABLED = coalesce( + process.env.DD_LLMOBS_ENABLED, + !!options.llmobs + ) + const DD_INSTRUMENTATION_INSTALL_ID = coalesce( process.env.DD_INSTRUMENTATION_INSTALL_ID, null @@ -320,6 +325,10 @@ class Config { // TODO: refactor this.apiKey = DD_API_KEY + this.llmobs = { + enabled: isTrue(DD_LLMOBS_ENABLED) + } + // sent in telemetry event app-started this.installSignature = { id: DD_INSTRUMENTATION_INSTALL_ID, @@ -497,6 +506,9 @@ class Config { this._setValue(defaults, 'isGitUploadEnabled', false) this._setValue(defaults, 'isIntelligentTestRunnerEnabled', false) this._setValue(defaults, 'isManualApiEnabled', false) + this._setValue(defaults, 'llmobs.agentlessEnabled', false) + this._setValue(defaults, 'llmobs.apiKey', undefined) + this._setValue(defaults, 'llmobs.mlApp', undefined) this._setValue(defaults, 'ciVisibilitySessionName', '') this._setValue(defaults, 'logInjection', false) this._setValue(defaults, 'lookup', undefined) @@ -594,6 +606,8 @@ class Config { DD_INSTRUMENTATION_TELEMETRY_ENABLED, DD_INSTRUMENTATION_CONFIG_ID, DD_LOGS_INJECTION, + DD_LLMOBS_AGENTLESS_ENABLED, + DD_LLMOBS_ML_APP, DD_OPENAI_LOGS_ENABLED, DD_OPENAI_SPAN_CHAR_LIMIT, DD_PROFILING_ENABLED, @@ -734,6 +748,8 @@ class Config { this._setArray(env, 'injectionEnabled', DD_INJECTION_ENABLED) this._setBoolean(env, 'isAzureFunction', getIsAzureFunction()) this._setBoolean(env, 'isGCPFunction', getIsGCPFunction()) + this._setBoolean(env, 'llmobs.agentlessEnabled', DD_LLMOBS_AGENTLESS_ENABLED) + this._setString(env, 'llmobs.mlApp', DD_LLMOBS_ML_APP) this._setBoolean(env, 'logInjection', DD_LOGS_INJECTION) // Requires an accompanying DD_APM_OBFUSCATION_MEMCACHED_KEEP_COMMAND=true in the agent this._setBoolean(env, 'memcachedCommandEnabled', DD_TRACE_MEMCACHED_COMMAND_ENABLED) @@ -902,6 +918,9 @@ class Config { } this._setString(opts, 'iast.telemetryVerbosity', options.iast && options.iast.telemetryVerbosity) this._setBoolean(opts, 'isCiVisibility', options.isCiVisibility) + this._setBoolean(opts, 'llmobs.agentlessEnabled', options.llmobs?.agentlessEnabled) + this._setString(opts, 'llmobs.apiKey', options.llmobs?.apiKey) + this._setString(opts, 'llmobs.mlApp', options.llmobs?.mlApp) this._setBoolean(opts, 'logInjection', options.logInjection) this._setString(opts, 'lookup', options.lookup) this._setBoolean(opts, 'openAiLogsEnabled', options.openAiLogsEnabled) From 2a3a3d350cb69b3cc5ed24770887e334c7806b31 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Tue, 17 Sep 2024 15:04:58 -0400 Subject: [PATCH 2/6] update tests --- packages/dd-trace/test/config.spec.js | 69 +++++++++++++++++++++++++-- 1 file changed, 65 insertions(+), 4 deletions(-) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index e047ab74e9d..14bd109f0e9 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -261,6 +261,9 @@ describe('Config', () => { expect(config).to.have.nested.property('installSignature.id', null) expect(config).to.have.nested.property('installSignature.time', null) expect(config).to.have.nested.property('installSignature.type', null) + expect(config).to.have.nested.property('llmobs.mlApp', undefined) + expect(config).to.have.nested.property('llmobs.agentlessEnabled', false) + expect(config).to.have.nested.property('llmobs.apiKey', undefined) expect(updateConfig).to.be.calledOnce @@ -323,6 +326,9 @@ describe('Config', () => { { name: 'isGitUploadEnabled', value: false, origin: 'default' }, { name: 'isIntelligentTestRunnerEnabled', value: false, origin: 'default' }, { name: 'isManualApiEnabled', value: false, origin: 'default' }, + { name: 'llmobs.agentlessEnabled', value: false, origin: 'default' }, + { name: 'llmobs.apiKey', value: undefined, origin: 'default' }, + { name: 'llmobs.mlApp', value: undefined, origin: 'default' }, { name: 'ciVisibilitySessionName', value: '', origin: 'default' }, { name: 'logInjection', value: false, origin: 'default' }, { name: 'lookup', value: undefined, origin: 'default' }, @@ -490,6 +496,8 @@ describe('Config', () => { process.env.DD_INSTRUMENTATION_INSTALL_TYPE = 'k8s_single_step' process.env.DD_INSTRUMENTATION_INSTALL_TIME = '1703188212' process.env.DD_INSTRUMENTATION_CONFIG_ID = 'abcdef123' + process.env.DD_LLMOBS_AGENTLESS_ENABLED = 'true' + process.env.DD_LLMOBS_ML_APP = 'myMlApp' // required if we want to check updates to config.debug and config.logLevel which is fetched from logger reloadLoggerAndConfig() @@ -584,6 +592,8 @@ describe('Config', () => { type: 'k8s_single_step', time: '1703188212' }) + expect(config).to.have.nested.property('llmobs.mlApp', 'myMlApp') + expect(config).to.have.nested.property('llmobs.agentlessEnabled', true) expect(updateConfig).to.be.calledOnce @@ -647,7 +657,9 @@ describe('Config', () => { { name: 'traceId128BitGenerationEnabled', value: true, origin: 'env_var' }, { name: 'traceId128BitLoggingEnabled', value: true, origin: 'env_var' }, { name: 'tracing', value: false, origin: 'env_var' }, - { name: 'version', value: '1.0.0', origin: 'env_var' } + { name: 'version', value: '1.0.0', origin: 'env_var' }, + { name: 'llmobs.mlApp', value: 'myMlApp', origin: 'env_var' }, + { name: 'llmobs.agentlessEnabled', value: true, origin: 'env_var' } ]) }) @@ -793,7 +805,12 @@ describe('Config', () => { pollInterval: 42 }, traceId128BitGenerationEnabled: true, - traceId128BitLoggingEnabled: true + traceId128BitLoggingEnabled: true, + llmobs: { + mlApp: 'myMlApp', + agentlessEnabled: true, + apiKey: 'myApiKey' + } }) expect(config).to.have.property('protocolVersion', '0.5') @@ -866,6 +883,9 @@ describe('Config', () => { a: 'aa', b: 'bb' }) + expect(config).to.have.nested.property('llmobs.mlApp', 'myMlApp') + expect(config).to.have.nested.property('llmobs.agentlessEnabled', true) + expect(config).to.have.nested.property('llmobs.apiKey', 'myApiKey') expect(updateConfig).to.be.calledOnce @@ -911,7 +931,10 @@ describe('Config', () => { { name: 'stats.enabled', value: false, origin: 'calculated' }, { name: 'traceId128BitGenerationEnabled', value: true, origin: 'code' }, { name: 'traceId128BitLoggingEnabled', value: true, origin: 'code' }, - { name: 'version', value: '0.1.0', origin: 'code' } + { name: 'version', value: '0.1.0', origin: 'code' }, + { name: 'llmobs.mlApp', value: 'myMlApp', origin: 'code' }, + { name: 'llmobs.agentlessEnabled', value: true, origin: 'code' }, + { name: 'llmobs.apiKey', value: 'myApiKey', origin: 'code' } ]) }) @@ -1085,6 +1108,8 @@ describe('Config', () => { process.env.DD_IAST_REDACTION_VALUE_PATTERN = 'value_pattern_to_be_overriden_by_options' process.env.DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED = 'true' process.env.DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED = 'true' + process.env.DD_LLMOBS_ML_APP = 'myMlApp' + process.env.DD_LLMOBS_AGENTLESS_ENABLED = 'true' const config = new Config({ protocolVersion: '0.5', @@ -1162,7 +1187,11 @@ describe('Config', () => { pollInterval: 42 }, traceId128BitGenerationEnabled: false, - traceId128BitLoggingEnabled: false + traceId128BitLoggingEnabled: false, + llmobs: { + mlApp: 'myOtherMlApp', + agentlessEnabled: false + } }) expect(config).to.have.property('protocolVersion', '0.5') @@ -1221,6 +1250,8 @@ describe('Config', () => { expect(config).to.have.nested.property('iast.redactionEnabled', true) expect(config).to.have.nested.property('iast.redactionNamePattern', 'REDACTION_NAME_PATTERN') expect(config).to.have.nested.property('iast.redactionValuePattern', 'REDACTION_VALUE_PATTERN') + expect(config).to.have.nested.property('llmobs.mlApp', 'myOtherMlApp') + expect(config).to.have.nested.property('llmobs.agentlessEnabled', false) }) it('should give priority to non-experimental options', () => { @@ -1979,6 +2010,36 @@ describe('Config', () => { }) }) + context('llmobs config', () => { + it('should disable llmobs by default', () => { + const config = new Config() + expect(config.llmobs.enabled).to.be.false + }) + + it('should enable llmobs if DD_LLMOBS_ENABLED is set to true', () => { + process.env.DD_LLMOBS_ENABLED = 'true' + const config = new Config() + expect(config.llmobs.enabled).to.be.true + }) + + it('should disable llmobs if DD_LLMOBS_ENABLED is set to false', () => { + process.env.DD_LLMOBS_ENABLED = 'false' + const config = new Config() + expect(config.llmobs.enabled).to.be.false + }) + + it('should enable llmobs with options and DD_LLMOBS_ENABLED is not set', () => { + const config = new Config({ llmobs: {} }) + expect(config.llmobs.enabled).to.be.true + }) + + it('should have DD_LLMOBS_ENABLED take priority over options', () => { + process.env.DD_LLMOBS_ENABLED = 'false' + const config = new Config({ llmobs: { enabled: true } }) + expect(config.llmobs.enabled).to.be.false + }) + }) + it('should sanitize values for API Security sampling between 0 and 1', () => { expect(new Config({ appsec: { From 8fd4912c4c710799c8d12ee5a47a975a3b44a799 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Tue, 17 Sep 2024 16:55:00 -0400 Subject: [PATCH 3/6] try and address approach to setting llmobs.enabled --- packages/dd-trace/src/config.js | 25 +++++++++++++------------ packages/dd-trace/test/config.spec.js | 1 + 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index e17a0dccf10..bdc0945d943 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -284,11 +284,6 @@ class Config { options.appsec = {} } - const DD_LLMOBS_ENABLED = coalesce( - process.env.DD_LLMOBS_ENABLED, - !!options.llmobs - ) - const DD_INSTRUMENTATION_INSTALL_ID = coalesce( process.env.DD_INSTRUMENTATION_INSTALL_ID, null @@ -325,10 +320,6 @@ class Config { // TODO: refactor this.apiKey = DD_API_KEY - this.llmobs = { - enabled: isTrue(DD_LLMOBS_ENABLED) - } - // sent in telemetry event app-started this.installSignature = { id: DD_INSTRUMENTATION_INSTALL_ID, @@ -348,7 +339,7 @@ class Config { this._applyDefaults() this._applyEnvironment() this._applyOptions(options) - this._applyCalculated() + this._applyCalculated(options) this._applyRemote({}) this._merge() @@ -408,7 +399,7 @@ class Config { } // TODO: test - this._applyCalculated() + this._applyCalculated(options) this._merge() } @@ -434,6 +425,13 @@ class Config { return inAWSLambda || isGCPFunction || isAzureFunction } + _isLLMObsEnabled (options = {}) { + return coalesce( + process.env.DD_LLMOBS_ENABLED, + !!options.llmobs + ) + } + // for _merge to work, every config value must have a default value _applyDefaults () { const { @@ -508,6 +506,7 @@ class Config { this._setValue(defaults, 'isManualApiEnabled', false) this._setValue(defaults, 'llmobs.agentlessEnabled', false) this._setValue(defaults, 'llmobs.apiKey', undefined) + this._setBoolean(defaults, 'llmobs.enabled', false) this._setValue(defaults, 'llmobs.mlApp', undefined) this._setValue(defaults, 'ciVisibilitySessionName', '') this._setValue(defaults, 'logInjection', false) @@ -1046,7 +1045,7 @@ class Config { } // handles values calculated from a mixture of options and env vars - _applyCalculated () { + _applyCalculated (options) { const calc = setHiddenProperty(this, '_calculated', {}) const { @@ -1090,6 +1089,8 @@ class Config { calc['tracePropagationStyle.inject'] = calc['tracePropagationStyle.inject'] || defaultPropagationStyle calc['tracePropagationStyle.extract'] = calc['tracePropagationStyle.extract'] || defaultPropagationStyle } + + this._setBoolean(calc, 'llmobs.enabled', this._isLLMObsEnabled(options)) } _applyRemote (options) { diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 14bd109f0e9..c4ec6faf5b9 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -264,6 +264,7 @@ describe('Config', () => { expect(config).to.have.nested.property('llmobs.mlApp', undefined) expect(config).to.have.nested.property('llmobs.agentlessEnabled', false) expect(config).to.have.nested.property('llmobs.apiKey', undefined) + expect(config).to.have.nested.property('llmobs.enabled', false) expect(updateConfig).to.be.calledOnce From b85e7fef3fbed933c8a1b65f3ab9fb472d2321a9 Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Tue, 17 Sep 2024 17:19:46 -0400 Subject: [PATCH 4/6] use this._optionsArg instead of options --- packages/dd-trace/src/config.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index bdc0945d943..1ad41024396 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -339,7 +339,7 @@ class Config { this._applyDefaults() this._applyEnvironment() this._applyOptions(options) - this._applyCalculated(options) + this._applyCalculated() this._applyRemote({}) this._merge() @@ -399,7 +399,7 @@ class Config { } // TODO: test - this._applyCalculated(options) + this._applyCalculated() this._merge() } @@ -425,10 +425,10 @@ class Config { return inAWSLambda || isGCPFunction || isAzureFunction } - _isLLMObsEnabled (options = {}) { + _isLLMObsEnabled () { return coalesce( process.env.DD_LLMOBS_ENABLED, - !!options.llmobs + !!this._optionsArg.llmobs ) } @@ -1045,7 +1045,7 @@ class Config { } // handles values calculated from a mixture of options and env vars - _applyCalculated (options) { + _applyCalculated () { const calc = setHiddenProperty(this, '_calculated', {}) const { @@ -1090,7 +1090,7 @@ class Config { calc['tracePropagationStyle.extract'] = calc['tracePropagationStyle.extract'] || defaultPropagationStyle } - this._setBoolean(calc, 'llmobs.enabled', this._isLLMObsEnabled(options)) + this._setBoolean(calc, 'llmobs.enabled', this._isLLMObsEnabled()) } _applyRemote (options) { From 6e8f039a8c590b14ca995eeedae818e47d3353fb Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Wed, 18 Sep 2024 09:15:48 -0400 Subject: [PATCH 5/6] change to _setValue --- packages/dd-trace/src/config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 1ad41024396..154b1a185d6 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -506,7 +506,7 @@ class Config { this._setValue(defaults, 'isManualApiEnabled', false) this._setValue(defaults, 'llmobs.agentlessEnabled', false) this._setValue(defaults, 'llmobs.apiKey', undefined) - this._setBoolean(defaults, 'llmobs.enabled', false) + this._setValue(defaults, 'llmobs.enabled', false) this._setValue(defaults, 'llmobs.mlApp', undefined) this._setValue(defaults, 'ciVisibilitySessionName', '') this._setValue(defaults, 'logInjection', false) From 3bd6bb55f588d0cdf9694ebfbf0f7d0e61f7a90b Mon Sep 17 00:00:00 2001 From: Sam Brenner Date: Wed, 18 Sep 2024 13:44:24 -0400 Subject: [PATCH 6/6] make sure config origin values are tracked properly for telemetry --- packages/dd-trace/src/config.js | 20 +++++++++++--------- packages/dd-trace/test/config.spec.js | 27 ++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 154b1a185d6..4c7b36f71fc 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -425,13 +425,6 @@ class Config { return inAWSLambda || isGCPFunction || isAzureFunction } - _isLLMObsEnabled () { - return coalesce( - process.env.DD_LLMOBS_ENABLED, - !!this._optionsArg.llmobs - ) - } - // for _merge to work, every config value must have a default value _applyDefaults () { const { @@ -606,6 +599,7 @@ class Config { DD_INSTRUMENTATION_CONFIG_ID, DD_LOGS_INJECTION, DD_LLMOBS_AGENTLESS_ENABLED, + DD_LLMOBS_ENABLED, DD_LLMOBS_ML_APP, DD_OPENAI_LOGS_ENABLED, DD_OPENAI_SPAN_CHAR_LIMIT, @@ -748,6 +742,7 @@ class Config { this._setBoolean(env, 'isAzureFunction', getIsAzureFunction()) this._setBoolean(env, 'isGCPFunction', getIsGCPFunction()) this._setBoolean(env, 'llmobs.agentlessEnabled', DD_LLMOBS_AGENTLESS_ENABLED) + this._setBoolean(env, 'llmobs.enabled', DD_LLMOBS_ENABLED) this._setString(env, 'llmobs.mlApp', DD_LLMOBS_ML_APP) this._setBoolean(env, 'logInjection', DD_LOGS_INJECTION) // Requires an accompanying DD_APM_OBFUSCATION_MEMCACHED_KEEP_COMMAND=true in the agent @@ -955,6 +950,15 @@ class Config { this._setBoolean(opts, 'traceId128BitGenerationEnabled', options.traceId128BitGenerationEnabled) this._setBoolean(opts, 'traceId128BitLoggingEnabled', options.traceId128BitLoggingEnabled) this._setString(opts, 'version', options.version || tags.version) + + // For LLMObs, we want the environment variable to take precedence over the options. + // This is reliant on environment config being set before options. + // This is to make sure the origins of each value are tracked appropriately for telemetry. + // We'll only set `llmobs.enabled` on the opts when it's not set on the environment, and options.llmobs is provided. + const llmobsEnabledEnv = this._env['llmobs.enabled'] + if (llmobsEnabledEnv == null && options.llmobs) { + this._setBoolean(opts, 'llmobs.enabled', !!options.llmobs) + } } _isCiVisibility () { @@ -1089,8 +1093,6 @@ class Config { calc['tracePropagationStyle.inject'] = calc['tracePropagationStyle.inject'] || defaultPropagationStyle calc['tracePropagationStyle.extract'] = calc['tracePropagationStyle.extract'] || defaultPropagationStyle } - - this._setBoolean(calc, 'llmobs.enabled', this._isLLMObsEnabled()) } _applyRemote (options) { diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index c4ec6faf5b9..956df5d2561 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -2015,29 +2015,54 @@ describe('Config', () => { it('should disable llmobs by default', () => { const config = new Config() expect(config.llmobs.enabled).to.be.false + + // check origin computation + expect(updateConfig.getCall(0).args[0]).to.deep.include({ + name: 'llmobs.enabled', value: false, origin: 'default' + }) }) it('should enable llmobs if DD_LLMOBS_ENABLED is set to true', () => { process.env.DD_LLMOBS_ENABLED = 'true' const config = new Config() expect(config.llmobs.enabled).to.be.true + + // check origin computation + expect(updateConfig.getCall(0).args[0]).to.deep.include({ + name: 'llmobs.enabled', value: true, origin: 'env_var' + }) }) it('should disable llmobs if DD_LLMOBS_ENABLED is set to false', () => { process.env.DD_LLMOBS_ENABLED = 'false' const config = new Config() expect(config.llmobs.enabled).to.be.false + + // check origin computation + expect(updateConfig.getCall(0).args[0]).to.deep.include({ + name: 'llmobs.enabled', value: false, origin: 'env_var' + }) }) it('should enable llmobs with options and DD_LLMOBS_ENABLED is not set', () => { const config = new Config({ llmobs: {} }) expect(config.llmobs.enabled).to.be.true + + // check origin computation + expect(updateConfig.getCall(0).args[0]).to.deep.include({ + name: 'llmobs.enabled', value: true, origin: 'code' + }) }) it('should have DD_LLMOBS_ENABLED take priority over options', () => { process.env.DD_LLMOBS_ENABLED = 'false' - const config = new Config({ llmobs: { enabled: true } }) + const config = new Config({ llmobs: {} }) expect(config.llmobs.enabled).to.be.false + + // check origin computation + expect(updateConfig.getCall(0).args[0]).to.deep.include({ + name: 'llmobs.enabled', value: false, origin: 'env_var' + }) }) })