-
Notifications
You must be signed in to change notification settings - Fork 77
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
[editor] add telemetry for more events #946
Conversation
1ce640b
to
a58dc87
Compare
46d5ce5
to
0dc7b23
Compare
772c9e3
to
bb72c10
Compare
As discussed offline, we want to enable telemetry in the local editor for the following events: - RUN_PROMPT_START - RUN_PROMPT_CANCEL - RUN_PROMPT_ERROR - RUN_PROMPT_SUCCESS This diff implements the logging telemetry for those events. - Send telemetry log events after dispatching rather than before, so that the client render action gets called first. - - This technically shouldn't matter since these are all async calls. - Note: This only implements logging for Local Editor. Next up is to add the datadog initialization to Gradio Workbook ## Testplan 1. yarn build -> run editor in 'prod' mode 2. run prompt, cancel, rerun to success. Check telemetry sent to datadog. https://github.com/lastmile-ai/aiconfig/assets/141073967/52043456-3682-4dd7-b95b-846ad7480019
bb72c10
to
b4beb0d
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.
Accepting to unblock, but would be good to add additional data to the events, mainly so that we can understand which models/parsers are being run most (and which ones are errored/cancelled).
Long-term we can also consider logging a unique id for each distinct run -> success/error/cancel flow so that we can add monitoring for when models/parsers are errored/cancelled at a higher-than-normal rate
|
||
const onPromptError = (message: string | null) => { | ||
dispatch({ | ||
type: "RUN_PROMPT_ERROR", | ||
promptId, | ||
message: message ?? undefined, | ||
}); | ||
logEventHandler?.("RUN_PROMPT_ERROR"); |
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 log the error message in the data as well? And maybe the prompt model
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 was thinking so too about the message but I think right now the error logs pass the entire aiconfig. wasn't sure if we want to be logging that since we said we don't want to log the config. Thoughts?
wdyt Should we pass in the message?
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.
If the message
includes the entire aiconfig, we shouldn't log that (and should probably fix to not do that). I was pretty sure it just returns an error message without config, though
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.
going to ship this and then investigate this (and make change as necessary ontop)
|
[editor] add telemetry for more events
As discussed offline, we want to enable telemetry in the local editor for the following events:
This diff implements the logging telemetry for those events.
Testplan
extra.telemetry.testplan.mov