-
Notifications
You must be signed in to change notification settings - Fork 81
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
[AIC-py][editor] server: persistent local cfg + telemetry flag, endpoints #869
Conversation
8022c7a
to
275f140
Compare
Can you add screenshot/video/text outputs from test plan? Thanks |
275f140
to
3e3df83
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.
Ok sorry, I accidentally left all my comments in #870 so please check there
3e3df83
to
e622b59
Compare
e622b59
to
f85b43c
Compare
fdede9b
to
ef15faf
Compare
…ints Test: - Start the server with and without flag and with no file, see that it gets created with correct contents - start the server with and without flag with existing file, with both true and false - hit both endpoints, check responses and that the file contents are changed https://github.com/lastmile-ai/aiconfig/assets/148090348/b36c3c00-c14b-4cde-8449-60386f2be1eb
ef15faf
to
ac90154
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.
discussed new changes offline, logic flow makes sense to me
with open(aiconfigrc_path, "x") as f: | ||
YAML().dump(DEFAULT_AICONFIGRC, f) |
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.
@rossdanlm, if we check that the file doesn't exist and then open it for writing, it's possible for another process to write the file in between those two steps.
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.
Ahhhhhhh, I see. Can you pls comment this in the code in #870 so that this is clear?
[AIC-py] cli rage This just tells the user how to open an issue. I don't want to do anything automatically because (a) user trust, and (b) it's much harder to implement without additional bugs. This is mostly bedside manner. If the user is raging, they are angry. Troll them a little bit to lighten the mood first. https://github.com/lastmile-ai/aiconfig/assets/148090348/2369306f-3af7-49f5-96bb-0ac9be323b51 --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/870). * #903 * __->__ #870 * #869
This diff builds on top of @rholinshead's starting point for telemetry diff. Most of that looked good to me so I didn't touch it beside @rossdanlm 's . - Building off of #869, if `allow_usage_data_sharing` is set to True, initialize Datadog Browser logging with a session ID. - Disabled telemetry in dev mode as per @rholinshead 's comment that the hot reload spams the logs with something like "datadog logging already initialized" - Moved the initialization logic away from`index.tsx` into `editor.tsx` so that it can be configurable. ## Testplan: 1. yarn build 2. run in "prod" mode 3. Edit AIconfig 4. Hit Save button -> validate datadog log sent repeat with aiconfig.rc set with false logging |<img width="963" alt="Screenshot 2024-01-12 at 7 24 33 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/c2f979df-327e-40c4-9f06-034531437a65"> | <img width="1455" alt="Screenshot 2024-01-12 at 7 23 51 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/edb022b5-4abd-4ddc-b9bb-97713d32e5dc"> | | ------------- | ------------- | | <img width="604" alt="Screenshot 2024-01-12 at 7 26 35 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/4a2e49a3-061e-404c-9681-13a15aca8e9c"> | -> No logs | I tried taking a video but datadog doesn't immediately update with logs which made the video too long to upload
This diff builds on top of @rholinshead's starting point for telemetry diff. Most of that looked good to me so I didn't touch it beside @rossdanlm 's . - Building off of #869, if `allow_usage_data_sharing` is set to True, initialize Datadog Browser logging with a session ID. - Disabled telemetry in dev mode as per @rholinshead 's comment that the hot reload spams the logs with something like "datadog logging already initialized" - Moved the initialization logic away from`index.tsx` into `editor.tsx` so that it can be configurable. ## Testplan: 1. yarn build 2. run in "prod" mode 3. Edit AIconfig 4. Hit Save button -> validate datadog log sent repeat with aiconfig.rc set with false logging |<img width="963" alt="Screenshot 2024-01-12 at 7 24 33 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/c2f979df-327e-40c4-9f06-034531437a65"> | <img width="1455" alt="Screenshot 2024-01-12 at 7 23 51 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/edb022b5-4abd-4ddc-b9bb-97713d32e5dc"> | | ------------- | ------------- | | <img width="604" alt="Screenshot 2024-01-12 at 7 26 35 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/4a2e49a3-061e-404c-9681-13a15aca8e9c"> | -> No logs | I tried taking a video but datadog doesn't immediately update with logs which made the video too long to upload
[editor][client] Enable Telemetry based on User Config settings This diff builds on top of @rholinshead's starting point for telemetry diff. Most of that looked good to me so I didn't touch it beside @rossdanlm 's . - Building off of #869, if `allow_usage_data_sharing` is set to True, initialize Datadog Browser logging with a session ID. - Disabled telemetry in dev mode as per @rholinshead 's comment that the hot reload spams the logs with something like "datadog logging already initialized" - Moved the initialization logic away from`index.tsx` into `editor.tsx` so that it can be configurable. ## Testplan: 1. yarn build 2. run in "prod" mode 3. Edit AIconfig 4. Hit Save button -> validate datadog log sent repeat with aiconfig.rc set with false logging |<img width="963" alt="Screenshot 2024-01-12 at 7 24 33 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/c2f979df-327e-40c4-9f06-034531437a65"> | <img width="1455" alt="Screenshot 2024-01-12 at 7 23 51 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/edb022b5-4abd-4ddc-b9bb-97713d32e5dc"> | | ------------- | ------------- | | <img width="604" alt="Screenshot 2024-01-12 at 7 26 35 PM" src="https://github.com/lastmile-ai/aiconfig/assets/141073967/4a2e49a3-061e-404c-9681-13a15aca8e9c"> | -> No logs | I tried taking a video but datadog doesn't immediately update with logs which made the video too long to upload --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/899). * __->__ #899 * #864
[AIC-py] cli rage part 2: issue draft Try to do a lot of stuff automatically. Run it and you'll see exactly what I mean :) `aiconfig rage` <img width="503" alt="Screenshot 2024-01-12 at 1 33 48 PM" src="https://github.com/lastmile-ai/aiconfig/assets/148090348/0415e458-681d-446f-b8ed-91b7a7c4bf1b"> --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/903). * __->__ #903 * #870 * #869
[AIC-py][editor] server: persistent local cfg + telemetry flag, endpoints
Test:
yaml.mov