Skip to content
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

Reduces telemetry sent #1237

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Reduces telemetry sent #1237

merged 2 commits into from
Nov 22, 2024

Conversation

skrawcz
Copy link
Collaborator

@skrawcz skrawcz commented Nov 22, 2024

reduces telemetry

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 574501d in 11 seconds

More details
  • Looked at 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. hamilton/telemetry.py:54
  • Draft comment:
    Reducing MAX_COUNT_SESSION from 100 to 10 aligns with the goal of reducing telemetry data sent. Ensure this change is intended and tested for any potential impact on data collection.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change in MAX_COUNT_SESSION from 100 to 10 is consistent with the PR's goal to reduce telemetry. This change will limit the number of events collected per session, effectively reducing the amount of telemetry data sent.
2. hamilton/telemetry.py:155
  • Draft comment:
    Avoid hardcoding API keys in the code. Consider using environment variables or a secure vault to manage sensitive information. This applies to the API_KEY variable.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_nFgvM4I8tBgfqIj4


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Need to set event count to higher since it seems to impact running tests.
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 5bd0151 in 30 seconds

More details
  • Looked at 44 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. tests/test_telemetry.py:13
  • Draft comment:
    Setting telemetry.MAX_COUNT_SESSION = 100 globally can affect other tests. Consider using a fixture to set and reset this value to avoid side effects.
  • Reason this comment was not posted:
    Marked as duplicate.
2. tests/test_hamilton_driver.py:237
  • Draft comment:
    Avoid hardcoding sensitive information like API keys in the codebase. Consider using environment variables or a secure vault to store them.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_ukyVQKf9eBqpEyPR


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

tests/test_hamilton_driver.py Show resolved Hide resolved
@skrawcz skrawcz merged commit f818bb9 into main Nov 22, 2024
24 checks passed
@skrawcz skrawcz deleted the reduce_telemetry branch November 22, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant