-
Notifications
You must be signed in to change notification settings - Fork 79
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
Interacting Telemetry [2/n]: Hook up mutable AIConfig events to datadog logger #1220
Conversation
65b08fd
to
5fc667a
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.
LGTM, but let's also log the model name when updating the model
@@ -452,6 +456,7 @@ function AIConfigEditorBase({ | |||
id: promptId, | |||
modelName: newModel, | |||
}); | |||
logEventHandler?.("UPDATE_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.
Can we also include the model name for better insight into what models are being used?:
logEventHandler?.("UPDATE_PROMPT_MODEL", {model: newModel});
| "SET_NAME" | ||
| "SET_DESCRIPTION" |
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.
nit, flip for alphabetical
…og logger This builds upon the PR from https://github.com/lastmile-ai/gradio-workbook/pull/184 where we defined the `session_id_manual` param for each session. Now we're just attaching this to all events that have the ability to change the AIConfig (we already had the run command, add prompt, download and share buttons). Note that for all the text-based ones, these fire every time we do text input chnage, so it will disproportionally spam the loggers, but we can adjust sample rate for those as a P1 in the future if needed. I heard overall that storage is not a concern for us at the moment, so feel it's fine to do this ## Test Plan Go to the `aiconfig/python/src/aiconfig/editor/client` directory and run this command ``` rm -rf node_modules && yarn && yarn build ``` Then from the `aiconfig` top-level directory, run this ``` aiconfig_path=./cookbooks/Gradio/huggingface.aiconfig.json parsers_path=./cookbooks/Gradio/aiconfig_model_registry.py aiconfig edit --aiconfig-path=$aiconfig_path --server-port=8080 --server-mode=debug_servers --parsers-module-path=$parsers_path ``` Comment out the if-else blocks from https://github.com/lastmile-ai/aiconfig/blob/601eaf64b8b1af5d7172457da2433d6bfdad1d09/python/src/aiconfig/editor/client/src/LocalEditor.tsx#L59-L62 <img width="1089" alt="Screenshot 2024-02-12 at 22 43 29" src="https://github.com/lastmile-ai/aiconfig/assets/151060367/f64ba580-03d0-4318-a334-e58aebb66882">
Interacting Telemetry [2/n]: Hook up mutable AIConfig events to datadog logger
This builds upon the PR from https://github.com/lastmile-ai/gradio-workbook/pull/184 where we defined the
session_id_manual
param for each session.Now we're just attaching this to all events that have the ability to change the AIConfig (we already had the run command, add prompt, download and share buttons). Note that for all the text-based ones, these fire every time we do text input chnage, so it will disproportionally spam the loggers, but we can adjust sample rate for those as a P1 in the future if needed. I heard overall that storage is not a concern for us at the moment, so feel it's fine to do this
Test Plan
Go to the
aiconfig/python/src/aiconfig/editor/client
directory and run this commandThen from the
aiconfig
top-level directory, run thisComment out the if-else blocks from
aiconfig/python/src/aiconfig/editor/client/src/LocalEditor.tsx
Lines 59 to 62 in 601eaf6