-
Notifications
You must be signed in to change notification settings - Fork 474
chore(llmobs): tli add apm trace #13466
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
Conversation
|
|
75a965a to
af97999
Compare
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 245 ± 4 ms. The average import time from base is: 242 ± 2 ms. The import time difference between this PR and base is: 3.3 ± 0.1 ms. Import time breakdownThe following import paths have grown:
|
BenchmarksBenchmark execution time: 2025-05-30 21:51:06 Comparing candidate commit 8acddb0 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 503 metrics, 5 unstable metrics. |
|
first time really playing with sdk so would also appreciate guidance on best practices/any additional tests. nervous ill break somethings so any qa approaches i wouldn't think of please let me know |
yshapiro-57
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have taken a look at this PR and left comments, feel free to address only those that make sense to you, @jsimpher.
I am doing a low-level line-by-line review here, probably similar to what Cursor would have done. I don't have a clear picture in my head of how different components of this code fit together; for example, if there was a file that was supposed to be changed but was not, I am not going to catch that in my review. So if you are able to please take a look at well, @sabrenner, I will really appreciate it.
yshapiro-57
left a comment
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.
Let me re-approve this now to indicate that my comments have all been addressed. You may want to take a look at the CI errors while we wait for Sam to give this a look as well.
sabrenner
left a comment
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.
looks great!! just doing some manual QAing and might add some other cleanup comments after but i think the approach is solid 😎
sabrenner
left a comment
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.
finishing up QAing but did a quick style pass again, just a couple thoughts
https://docs.google.com/document/d/1ZkKRU0JfljPiwLH1mXaAYRpFaEMFpRGKc5axOjo8mjo/edit?tab=t.0
The goal here is to separate the concept of an llmobs trace id from that of an apm trace id, which will have value in trace level insights, as well as resolving some ui issues around multiple llmobs traces sharing a single trace id.
This is accomplished by adding an llmobs trace id tag to the span, passing it down from parent to child, and ultimately using it instead of trace_id to fill the llmobs_span['trace_id'] slot. the original apm trace id now to be stored in
._dd.apm_trace_id.QA kind of important here, relatively high blast radius, could ruin trace grouping/heirarchy
Checklist
Reviewer Checklist