-
Notifications
You must be signed in to change notification settings - Fork 455
chore(llmobs): make expected output optional #14208
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
|
8e753b3
to
35a8cf0
Compare
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 268 ± 3 ms. The average import time from base is: 270 ± 3 ms. The import time difference between this PR and base is: -2.5 ± 0.1 ms. Import time breakdownThe following import paths have shrunk:
|
Performance SLOsCandidate: gary/mlob-3483 (745b901) 🔵 No Baseline Data (24 suites)🔵 coreapiscenario - 12/12 (2 unstable)🔵 No baseline data available for this suite
|
1938fa5
to
75f948e
Compare
75f948e
to
6ab758e
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.
One quick comment but will review with fresher eyes tomorrow 😎
d7d4798
to
51fb500
Compare
Still need to check why the CI is failing for tests. The logic seemed good for me. |
f729b2d
to
625960a
Compare
625960a
to
67e79cc
Compare
BenchmarksBenchmark execution time: 2025-08-06 17:16:22 Comparing candidate commit 625960a in PR branch Found 0 performance improvements and 6 performance regressions! Performance is the same for 553 metrics, 9 unstable metrics. scenario:httppropagationinject-ids_only
scenario:httppropagationinject-with_dd_origin
scenario:httppropagationinject-with_tags_invalid
scenario:iastaspects-translate_aspect
scenario:telemetryaddmetric-1-distribution-metric-1-times
scenario:telemetryaddmetric-flush-1-metric
|
32b89bf
to
a3c10cf
Compare
a3c10cf
to
13124bd
Compare
@gary-huang is there a particular piece of this that you'd like my input on? All of these files are owned by another team that has a lot more context about them than I do. |
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.
Last comments! I'd like to get rid of the duplicate functions if possible
fix case where updating a record with expected_output to no expected_output (null)
Co-authored-by: Yahya Mouman <[email protected]> Co-authored-by: Yun Kim <[email protected]>
c79e6ac
to
7609baa
Compare
7609baa
to
745b901
Compare
not particularly, you requested changes earlier so feel free to take another look if you'd like :P |
pulling dataset with empty "expected_column" fails before


before
after
creating dataset


before
after
append, update, and delete would result in errors on push

All those cases succeed after this change
A large dataset pull has been tested to work with the extended timeout, we will bump the timeout as needed, and possibly make it configurable in the future
Checklist
Reviewer Checklist