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 max events to 100 #731

Merged
merged 1 commit into from
Mar 3, 2024
Merged

Reduces max events to 100 #731

merged 1 commit into from
Mar 3, 2024

Conversation

skrawcz
Copy link
Collaborator

@skrawcz skrawcz commented Mar 3, 2024

To reduce overall number of total events
we collect per python process.

Changes

  • telemetry.py

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.

Ellipsis 🚀 This PR description was created by Ellipsis for commit a161238.

Summary:

This PR reduces the maximum number of events collected per Python process from 1000 to 100 in telemetry.py.

Key points:

  • Reduced MAX_COUNT_SESSION from 1000 to 100 in telemetry.py

Generated with ❤️ by ellipsis.dev

To reduce overall number of total events
we collect per python process.
@skrawcz skrawcz requested a review from elijahbenizzy March 3, 2024 06:02
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 the entire pull request up to a304fff
  • Looked at 14 lines of code in 1 files
  • Took 26 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. hamilton/telemetry.py:54:
  • Assessed confidence : 50%
  • Comment:
    This change reduces the max number of events collected per python process from 1000 to 100. While this might help in reducing the overall number of total events, it could potentially lead to loss of important telemetry data in long-running or complex processes. Please ensure that this change has been adequately tested and that the potential loss of telemetry data is acceptable.
  • Reasoning:
    The change in the PR is straightforward and doesn't seem to introduce any bugs. However, it's important to consider the implications of this change. Reducing the max number of events collected per python process from 1000 to 100 could potentially lead to loss of important telemetry data, especially in long-running or complex processes. This could impact the ability to monitor and debug the system effectively. It's also unclear whether this change has been tested adequately, as the PR description doesn't provide any details about testing.

Workflow ID: wflow_4fJPUtdSVXS4Krfq


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

LGTN

@elijahbenizzy elijahbenizzy merged commit a161238 into main Mar 3, 2024
23 checks passed
@elijahbenizzy elijahbenizzy deleted the reduce_max_event_count branch March 3, 2024 06:23
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!

  • Performed an incremental review on a161238
  • Looked at 13 lines of code in 1 files
  • Took 45 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. hamilton/telemetry.py:54:
  • Assessed confidence : 33%
  • Comment:
    This change reduces the maximum number of events collected per Python process from 1000 to 100. While this doesn't introduce any bugs, it's important to consider the implications. This could potentially limit the amount of telemetry data collected, which could impact the ability to monitor and improve the system. It could also impact any systems or processes that rely on or expect a certain volume of telemetry data.
  • Reasoning:
    The change in the PR is straightforward and doesn't seem to introduce any bugs. However, it's important to consider the implications of this change. Reducing the maximum number of events collected per Python process from 1000 to 100 could potentially limit the amount of telemetry data collected, which could impact the ability to monitor and improve the system. It's also worth noting that this change could potentially impact any systems or processes that rely on or expect a certain volume of telemetry data.

Workflow ID: wflow_hhIKrbaFHwn7YXLX


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

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.

2 participants