-
Notifications
You must be signed in to change notification settings - Fork 383
fix(llmobs): use startup or forced error logs instead of throwing for invalid LLMObs initializations #7885
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?
fix(llmobs): use startup or forced error logs instead of throwing for invalid LLMObs initializations #7885
Changes from all commits
1e0ab19
a93b518
b3569ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,14 @@ function logAgentError (agentError) { | |
| } | ||
| } | ||
|
|
||
| function logGenericError (message) { | ||
| if (!config?.startupLogs) { | ||
| return | ||
| } | ||
|
|
||
| warn('DATADOG TRACER DIAGNOSTIC - Generic Error: ' + message) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Startup log is meant to be a single line message and having two separate entries is changing that contract. I guess what we need is a state to wait for the agent strategy to be ready. That might conflict with other expectations though. @rochdev what would be your take on that?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooh ok, i saw we had something for an agent error log and was just trying something similar.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that multiple startup logs are now a thing, but I'm not sure if they have to respect a specific structure. I think @bm1549 has the most recent context.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rochdev is correct - multiple startup logs are a thing now and are used to de-couple different concerns, e.g. an initial That being said, this seems to point to more of an invalid configuration state since this simplifies to @BridgeAR have we considered having invalid state as part of the config system so that we warn users if they provide something like this? |
||
| } | ||
|
|
||
| /** | ||
| * Returns config info without integrations (used by startupLog). | ||
| * @returns {Record<string, unknown>} | ||
|
|
@@ -143,4 +151,5 @@ module.exports = { | |
| setSamplingRules, | ||
| tracerInfo, | ||
| errors, | ||
| logGenericError, | ||
| } | ||
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 am unsure if I would like to log if startupLogs is deactivated when we activate that by default in the future. I think in that case we should not log.