-
Notifications
You must be signed in to change notification settings - Fork 311
Add support for instrumenting the OpenAI Client #9284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Code coverage: total 57.09%, patch 100.00% (view details) This comment will be updated automatically if new data arrives.🔗 Commit SHA: 298f31f | Docs | Was this helpful? Give us feedback! |
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 13 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.52.0-SNAPSHOT~298f31f1de, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.043 s) : 0, 1042589
Total [baseline] (10.713 s) : 0, 10713027
Agent [candidate] (1.056 s) : 0, 1055804
Total [candidate] (10.79 s) : 0, 10789977
section appsec
Agent [baseline] (1.221 s) : 0, 1221168
Total [baseline] (10.689 s) : 0, 10689457
Agent [candidate] (1.219 s) : 0, 1219088
Total [candidate] (10.762 s) : 0, 10762333
section iast
Agent [baseline] (1.176 s) : 0, 1175991
Total [baseline] (10.909 s) : 0, 10909244
Agent [candidate] (1.176 s) : 0, 1175874
Total [candidate] (10.907 s) : 0, 10907372
section profiling
Agent [baseline] (1.193 s) : 0, 1192698
Total [baseline] (10.802 s) : 0, 10801538
Agent [candidate] (1.198 s) : 0, 1198225
Total [candidate] (10.897 s) : 0, 10896662
gantt
title petclinic - break down per module: candidate=1.52.0-SNAPSHOT~298f31f1de, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.42 ms) : 0, 1420
crashtracking [candidate] (1.444 ms) : 0, 1444
BytebuddyAgent [baseline] (730.076 ms) : 0, 730076
BytebuddyAgent [candidate] (739.012 ms) : 0, 739012
GlobalTracer [baseline] (241.412 ms) : 0, 241412
GlobalTracer [candidate] (243.631 ms) : 0, 243631
AppSec [baseline] (30.02 ms) : 0, 30020
AppSec [candidate] (30.494 ms) : 0, 30494
Debugger [baseline] (6.003 ms) : 0, 6003
Debugger [candidate] (6.083 ms) : 0, 6083
Remote Config [baseline] (651.574 µs) : 0, 652
Remote Config [candidate] (659.068 µs) : 0, 659
Telemetry [baseline] (12.099 ms) : 0, 12099
Telemetry [candidate] (13.348 ms) : 0, 13348
section appsec
crashtracking [baseline] (1.43 ms) : 0, 1430
crashtracking [candidate] (1.42 ms) : 0, 1420
BytebuddyAgent [baseline] (755.597 ms) : 0, 755597
BytebuddyAgent [candidate] (753.357 ms) : 0, 753357
GlobalTracer [baseline] (234.18 ms) : 0, 234180
GlobalTracer [candidate] (234.271 ms) : 0, 234271
IAST [baseline] (23.634 ms) : 0, 23634
IAST [candidate] (23.634 ms) : 0, 23634
AppSec [baseline] (169.162 ms) : 0, 169162
AppSec [candidate] (170.13 ms) : 0, 170130
Debugger [baseline] (6.382 ms) : 0, 6382
Debugger [candidate] (6.42 ms) : 0, 6420
Remote Config [baseline] (620.282 µs) : 0, 620
Remote Config [candidate] (609.139 µs) : 0, 609
Telemetry [baseline] (9.124 ms) : 0, 9124
Telemetry [candidate] (8.23 ms) : 0, 8230
section iast
crashtracking [baseline] (1.427 ms) : 0, 1427
crashtracking [candidate] (1.426 ms) : 0, 1426
BytebuddyAgent [baseline] (849.406 ms) : 0, 849406
BytebuddyAgent [candidate] (848.885 ms) : 0, 848885
GlobalTracer [baseline] (232.088 ms) : 0, 232088
GlobalTracer [candidate] (232.376 ms) : 0, 232376
IAST [baseline] (27.476 ms) : 0, 27476
IAST [candidate] (30.669 ms) : 0, 30669
AppSec [baseline] (27.559 ms) : 0, 27559
AppSec [candidate] (26.224 ms) : 0, 26224
Debugger [baseline] (8.223 ms) : 0, 8223
Debugger [candidate] (6.611 ms) : 0, 6611
Remote Config [baseline] (581.944 µs) : 0, 582
Remote Config [candidate] (599.779 µs) : 0, 600
Telemetry [baseline] (8.211 ms) : 0, 8211
Telemetry [candidate] (8.153 ms) : 0, 8153
section profiling
crashtracking [baseline] (1.392 ms) : 0, 1392
crashtracking [candidate] (1.405 ms) : 0, 1405
BytebuddyAgent [baseline] (760.304 ms) : 0, 760304
BytebuddyAgent [candidate] (763.583 ms) : 0, 763583
GlobalTracer [baseline] (221.372 ms) : 0, 221372
GlobalTracer [candidate] (222.39 ms) : 0, 222390
AppSec [baseline] (29.75 ms) : 0, 29750
AppSec [candidate] (30.197 ms) : 0, 30197
Debugger [baseline] (6.316 ms) : 0, 6316
Debugger [candidate] (6.319 ms) : 0, 6319
Remote Config [baseline] (681.764 µs) : 0, 682
Remote Config [candidate] (680.087 µs) : 0, 680
Telemetry [baseline] (15.84 ms) : 0, 15840
Telemetry [candidate] (15.957 ms) : 0, 15957
ProfilingAgent [baseline] (107.65 ms) : 0, 107650
ProfilingAgent [candidate] (108.106 ms) : 0, 108106
Profiling [baseline] (108.305 ms) : 0, 108305
Profiling [candidate] (108.755 ms) : 0, 108755
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.52.0-SNAPSHOT~298f31f1de, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.044 s) : 0, 1043513
Total [baseline] (8.565 s) : 0, 8564622
Agent [candidate] (1.052 s) : 0, 1052456
Total [candidate] (8.574 s) : 0, 8573934
section iast
Agent [baseline] (1.177 s) : 0, 1177332
Total [baseline] (9.374 s) : 0, 9373931
Agent [candidate] (1.173 s) : 0, 1173185
Total [candidate] (9.298 s) : 0, 9297826
gantt
title insecure-bank - break down per module: candidate=1.52.0-SNAPSHOT~298f31f1de, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.428 ms) : 0, 1428
crashtracking [candidate] (1.451 ms) : 0, 1451
BytebuddyAgent [baseline] (730.672 ms) : 0, 730672
BytebuddyAgent [candidate] (737.921 ms) : 0, 737921
GlobalTracer [baseline] (241.15 ms) : 0, 241150
GlobalTracer [candidate] (242.371 ms) : 0, 242371
AppSec [baseline] (29.95 ms) : 0, 29950
AppSec [candidate] (30.226 ms) : 0, 30226
Debugger [baseline] (5.985 ms) : 0, 5985
Debugger [candidate] (6.059 ms) : 0, 6059
Remote Config [baseline] (648.188 µs) : 0, 648
Remote Config [candidate] (650.076 µs) : 0, 650
Telemetry [baseline] (12.787 ms) : 0, 12787
Telemetry [candidate] (12.718 ms) : 0, 12718
section iast
crashtracking [baseline] (1.439 ms) : 0, 1439
crashtracking [candidate] (1.424 ms) : 0, 1424
BytebuddyAgent [baseline] (849.707 ms) : 0, 849707
BytebuddyAgent [candidate] (847.496 ms) : 0, 847496
GlobalTracer [baseline] (233.234 ms) : 0, 233234
GlobalTracer [candidate] (230.892 ms) : 0, 230892
IAST [baseline] (30.368 ms) : 0, 30368
IAST [candidate] (26.665 ms) : 0, 26665
AppSec [baseline] (26.08 ms) : 0, 26080
AppSec [candidate] (30.199 ms) : 0, 30199
Debugger [baseline] (6.654 ms) : 0, 6654
Debugger [candidate] (6.723 ms) : 0, 6723
Remote Config [baseline] (595.797 µs) : 0, 596
Remote Config [candidate] (593.925 µs) : 0, 594
Telemetry [baseline] (8.371 ms) : 0, 8371
Telemetry [candidate] (8.205 ms) : 0, 8205
LoadParameters
See matching parameters
SummaryFound 3 performance improvements and 3 performance regressions! Performance is the same for 6 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~298f31f1de, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section baseline
no_agent (35.681 ms) : 35391, 35970
. : milestone, 35681,
appsec (47.899 ms) : 47478, 48319
. : milestone, 47899,
code_origins (45.36 ms) : 44965, 45755
. : milestone, 45360,
iast (43.308 ms) : 42929, 43688
. : milestone, 43308,
profiling (49.672 ms) : 49194, 50149
. : milestone, 49672,
tracing (43.547 ms) : 43192, 43903
. : milestone, 43547,
section candidate
no_agent (37.782 ms) : 37476, 38089
. : milestone, 37782,
appsec (46.425 ms) : 46022, 46827
. : milestone, 46425,
code_origins (45.565 ms) : 45157, 45973
. : milestone, 45565,
iast (46.402 ms) : 45991, 46813
. : milestone, 46402,
profiling (48.993 ms) : 48533, 49453
. : milestone, 48993,
tracing (42.255 ms) : 41902, 42607
. : milestone, 42255,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~298f31f1de, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section baseline
no_agent (4.523 ms) : 4471, 4574
. : milestone, 4523,
iast (9.719 ms) : 9563, 9875
. : milestone, 9719,
iast_FULL (13.96 ms) : 13686, 14235
. : milestone, 13960,
iast_GLOBAL (10.543 ms) : 10356, 10730
. : milestone, 10543,
profiling (8.729 ms) : 8586, 8872
. : milestone, 8729,
tracing (7.749 ms) : 7628, 7870
. : milestone, 7749,
section candidate
no_agent (4.324 ms) : 4276, 4372
. : milestone, 4324,
iast (9.549 ms) : 9383, 9715
. : milestone, 9549,
iast_FULL (14.158 ms) : 13878, 14438
. : milestone, 14158,
iast_GLOBAL (10.28 ms) : 10086, 10474
. : milestone, 10280,
profiling (9.11 ms) : 8957, 9263
. : milestone, 9110,
tracing (7.566 ms) : 7455, 7678
. : milestone, 7566,
DacapoParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 0 unstable metrics.
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~298f31f1de, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section baseline
no_agent (1.478 ms) : 1466, 1489
. : milestone, 1478,
appsec (3.618 ms) : 3406, 3830
. : milestone, 3618,
iast (2.213 ms) : 2150, 2276
. : milestone, 2213,
iast_GLOBAL (2.254 ms) : 2190, 2317
. : milestone, 2254,
profiling (2.046 ms) : 1995, 2096
. : milestone, 2046,
tracing (2.024 ms) : 1976, 2073
. : milestone, 2024,
section candidate
no_agent (1.478 ms) : 1466, 1489
. : milestone, 1478,
appsec (2.471 ms) : 2418, 2523
. : milestone, 2471,
iast (2.199 ms) : 2137, 2262
. : milestone, 2199,
iast_GLOBAL (2.254 ms) : 2191, 2317
. : milestone, 2254,
profiling (2.05 ms) : 2000, 2100
. : milestone, 2050,
tracing (2.024 ms) : 1976, 2072
. : milestone, 2024,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~298f31f1de, baseline=1.53.0-SNAPSHOT~ee43e5f19c
dateFormat X
axisFormat %s
section baseline
no_agent (14.932 s) : 14932000, 14932000
. : milestone, 14932000,
appsec (14.893 s) : 14893000, 14893000
. : milestone, 14893000,
iast (18.187 s) : 18187000, 18187000
. : milestone, 18187000,
iast_GLOBAL (18.039 s) : 18039000, 18039000
. : milestone, 18039000,
profiling (15.532 s) : 15532000, 15532000
. : milestone, 15532000,
tracing (14.85 s) : 14850000, 14850000
. : milestone, 14850000,
section candidate
no_agent (15.043 s) : 15043000, 15043000
. : milestone, 15043000,
appsec (14.886 s) : 14886000, 14886000
. : milestone, 14886000,
iast (18.97 s) : 18970000, 18970000
. : milestone, 18970000,
iast_GLOBAL (18.045 s) : 18045000, 18045000
. : milestone, 18045000,
profiling (15.682 s) : 15682000, 15682000
. : milestone, 15682000,
tracing (14.988 s) : 14988000, 14988000
. : milestone, 14988000,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments to address before merging.
...trumentation/openai/src/main/java/datadog/trace/instrumentation/openai/OpenAIClientInfo.java
Outdated
Show resolved
Hide resolved
...trumentation/openai/src/main/java/datadog/trace/instrumentation/openai/OpenAIClientInfo.java
Outdated
Show resolved
Hide resolved
settings.gradle
Outdated
@@ -430,6 +430,7 @@ include ':dd-java-agent:instrumentation:ognl-appsec' | |||
include ':dd-java-agent:instrumentation:opensearch' | |||
include ':dd-java-agent:instrumentation:opensearch:rest' | |||
include ':dd-java-agent:instrumentation:opensearch:transport' | |||
include ':dd-java-agent:instrumentation:openai' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Can you follow the layout described here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Not resolved. Made a suggestion to highlight the required change.
...src/main/java/datadog/trace/instrumentation/openai/ChatCompletionServiceInstrumentation.java
Outdated
Show resolved
Hide resolved
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
// Extract tool calls if present | ||
int callIndex = 0; | ||
for (ChatCompletionMessageToolCall call : toolCalls.get()) { | ||
span.setTag( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: as a room for optimisation those tags can be perhaps cached. the second one is also sharing a big part of the first one
|
||
public AgentScope startChatCompletionSpan(ChatCompletionCreateParams params) { | ||
AgentSpan span = startSpan(OPENAI_REQUEST); | ||
span.setTag("openai.request.endpoint", "/chat/completions"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: those could be stored as constants for a better reading
import com.openai.core.ClientOptions; | ||
import com.openai.credential.BearerTokenCredential; | ||
|
||
public class OpenAIClientInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the information collected in this class used?
llmObsSpan.setMetadata(responseMetadata); | ||
} | ||
} catch (Exception e) { | ||
java.util.Map<String, Object> errorMetadata = new java.util.HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map and HashMap should be imported
f810778
to
93a22fe
Compare
98fe251
to
ce288b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any tests for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comment about tracing instrumentation deactivation override, convention, possible dd-trace-ot, etc...
And where are the tests @AlexeyKuznetsov-DD asked for?
settings.gradle.kts
Outdated
@@ -459,6 +459,7 @@ include( | |||
":dd-java-agent:instrumentation:okhttp-2", | |||
":dd-java-agent:instrumentation:okhttp-3", | |||
":dd-java-agent:instrumentation:ognl-appsec", | |||
":dd-java-agent:instrumentation:openai-client", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
":dd-java-agent:instrumentation:openai-client", | |
":dd-java-agent:instrumentation:openai-java:openai-java-2.8", |
settings.gradle
Outdated
@@ -430,6 +430,7 @@ include ':dd-java-agent:instrumentation:ognl-appsec' | |||
include ':dd-java-agent:instrumentation:opensearch' | |||
include ':dd-java-agent:instrumentation:opensearch:rest' | |||
include ':dd-java-agent:instrumentation:opensearch:transport' | |||
include ':dd-java-agent:instrumentation:openai' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Not resolved. Made a suggestion to highlight the required change.
public class ChatCompletionServiceInstrumentation extends InstrumenterModule.Tracing | ||
implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice { | ||
public ChatCompletionServiceInstrumentation() { | ||
super("openai-client", "openai-java", "openai-2.8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Conventions
super("openai-client", "openai-java", "openai-2.8"); | |
super("openai-java", "openai-java-2.8", "openai-client"); |
|
||
@Override | ||
protected boolean defaultEnabled() { | ||
return InstrumenterConfig.get().isIntegrationEnabled(Collections.singleton("openai"), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: This should not override tracing instrumentation deactivation.
public class CompletionServiceInstrumentation extends InstrumenterModule.Tracing | ||
implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice { | ||
public CompletionServiceInstrumentation() { | ||
super("openai-client", "openai-java", "openai-2.8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Same
|
||
@Override | ||
protected boolean defaultEnabled() { | ||
return InstrumenterConfig.get().isIntegrationEnabled(Collections.singleton("openai"), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Same
public class OpenAIClientInstrumentation extends InstrumenterModule.Tracing | ||
implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { | ||
public OpenAIClientInstrumentation() { | ||
super("openai-client", "openai-java", "openai-2.8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Same
|
||
@Override | ||
protected boolean defaultEnabled() { | ||
return InstrumenterConfig.get().isIntegrationEnabled(Collections.singleton("openai"), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Same
dd-trace-core/build.gradle
Outdated
@@ -72,6 +72,8 @@ dependencies { | |||
implementation project(':utils:socket-utils') | |||
// for span exception debugging | |||
compileOnly project(':dd-java-agent:agent-debugger:debugger-bootstrap') | |||
// for llmobs | |||
compileOnly project(':dd-java-agent:agent-llmobs') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔨 issue: Will this work with dd-trace-ot
?
Product should not be added here
@@ -22,7 +22,7 @@ public class LLMObsSystem { | |||
|
|||
private static final String CUSTOM_MODEL_VAL = "custom"; | |||
|
|||
public static void start(Instrumentation inst, SharedCommunicationObjects sco) { | |||
public static void start(@Nullable Instrumentation inst, SharedCommunicationObjects sco) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 suggestion: If this arg is always null
and unused, why keeping it in the first place?
What Does This Do
Adds auto-instrumentation for the official openai java client.
Motivation
Additional Notes
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]