-
Notifications
You must be signed in to change notification settings - Fork 484
fix(llmobs): include missing tags and make SubmitEvaluationFromSpan more flexible #4050
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: main
Are you sure you want to change the base?
Conversation
|
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.
nice lgtm! just a couple noob Go questions 😄
internal/llmobs/llmobs.go
Outdated
} | ||
if cfg.AgentlessEnabled != nil { | ||
if !*cfg.AgentlessEnabled && !agentSupportsLLMObs { | ||
log.Warn("llmobs: DD_LLMOBS_AGENTLESS_ENABLED has been configured to false and the agent is not available or does not support 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.
should we return an error in this case? i don't think llmobs will be able to submit data in the scenario where there's no agent running and agentlessEnabled is explicitly set to false
, and maybe we wanna surface that to the user harsher than a warning log, but not sure of the protocols for doing that in the go tracer!
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.
hmm yes! this totally makes sense, I will change it 👍
…ode is explicitly setup
/merge |
View all feedbacks in Devflow UI.
This merge request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
devflow unqueued this merge request: It did not become mergeable within the expected time |
What does this PR do?
Fix some minor issues in the llmobs SDK highlighted by the shared testing suite:
session_id
in tags. It was currently only being included in the root field of the span event payload, but it should be present in both places.ddtrace.version
in evaluation metrics.Motivation
Correct/consistent implementation of the Go LLMObs SDK.
Reviewer's Checklist
./scripts/lint.sh
locally.Unsure? Have a question? Request a review!