-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: ingestion limits - add toggle feature #6430
base: develop
Are you sure you want to change the base?
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Reviewed everything up to 6f09478 in 1 minute and 39 seconds
More details
- Looked at
449
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:428
- Draft comment:
Consider refactoring the repeated logic for settingpayload.config
inhandleAddLimit
andhandleUpdateLimit
functions to improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The code inMultiIngestionSettings.tsx
has a repeated logic for setting thepayload.config
object in bothhandleAddLimit
andhandleUpdateLimit
functions. This can be refactored to improve code readability and maintainability.
2. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:477
- Draft comment:
Consider refactoring the repeated logic for checking undefined limits and settingpayload.config
inhandleAddLimit
andhandleUpdateLimit
functions to avoid code duplication. - Reason this comment was not posted:
Confidence changes required:50%
ThehandleAddLimit
andhandleUpdateLimit
functions inMultiIngestionSettings.tsx
have similar logic for checking undefined limits and setting the payload. This can be refactored to avoid code duplication.
3. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:522
- Draft comment:
The logic for setting theenabled
property based on the presence ofsize
is repeated. Consider extracting this logic into a utility function for better code reuse. - Reason this comment was not posted:
Confidence changes required:50%
InMultiIngestionSettings.tsx
, theenableEditLimitMode
function sets theenabled
property based on the presence ofsize
. This logic is repeated in multiple places and can be extracted into a utility function for better code reuse.
4. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:434
- Draft comment:
Ensure that the input values fordailyLimit
andsecondsLimit
are validated before conversion to prevent unexpected behavior. - Reason this comment was not posted:
Confidence changes required:50%
InMultiIngestionSettings.tsx
, thehandleAddLimit
andhandleUpdateLimit
functions usegbToBytes
conversion. This conversion logic is correct, but ensure that the input values are validated to prevent unexpected behavior.
5. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:484
- Draft comment:
Ensure that the input values fordailyLimit
andsecondsLimit
are validated before conversion to prevent unexpected behavior. - Reason this comment was not posted:
Confidence changes required:50%
InMultiIngestionSettings.tsx
, thehandleAddLimit
andhandleUpdateLimit
functions usegbToBytes
conversion. This conversion logic is correct, but ensure that the input values are validated to prevent unexpected behavior.
6. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:446
- Draft comment:
Ensure that the input values fordailyLimit
andsecondsLimit
are validated before conversion to prevent unexpected behavior. - Reason this comment was not posted:
Confidence changes required:50%
InMultiIngestionSettings.tsx
, thehandleAddLimit
andhandleUpdateLimit
functions usegbToBytes
conversion. This conversion logic is correct, but ensure that the input values are validated to prevent unexpected behavior.
7. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:495
- Draft comment:
Ensure that the input values fordailyLimit
andsecondsLimit
are validated before conversion to prevent unexpected behavior. - Reason this comment was not posted:
Confidence changes required:50%
InMultiIngestionSettings.tsx
, thehandleAddLimit
andhandleUpdateLimit
functions usegbToBytes
conversion. This conversion logic is correct, but ensure that the input values are validated to prevent unexpected behavior.
8. frontend/src/container/IngestionSettings/IngestionSettings.styles.scss:390
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values likevar(--bg-forest-400)
to maintain consistency in design and theming. This is also applicable to other instances in the file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code usesvar(--bg-forest-400)
, which appears to be a CSS variable. CSS variables are often used as design tokens or predefined constants. The comment seems to misunderstand the use of CSS variables as design tokens. The file consistently uses CSS variables, indicating thatvar(--bg-forest-400)
is likely a predefined constant.
I might be missing the context of whethervar(--bg-forest-400)
is indeed a predefined constant or a hardcoded value. However, given the consistent use of CSS variables, it's reasonable to assume it is predefined.
The consistent use of CSS variables throughout the file supports the assumption thatvar(--bg-forest-400)
is a predefined constant, making the comment unnecessary.
The comment should be deleted becausevar(--bg-forest-400)
is likely a predefined color constant, and the comment misunderstands the use of CSS variables as design tokens.
9. frontend/src/container/IngestionSettings/MultiIngestionSettings.tsx:408
-
Draft comment:
This function duplicates the functionality ofhideDeleteScheduleModal
inPlannedDowntimeDeleteModal.tsx
. Consider refactoring to use a common utility function. -
function
hideDeleteScheduleModal
(PlannedDowntimeDeleteModal.tsx) -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a potential code duplication, but the functions are very simple and specific to their respective components. Creating a utility function for such a simple operation might not be justified. The comment does not provide strong evidence that a refactor is necessary or beneficial.
The comment assumes that a utility function is needed without considering the context of each component. The functions are simple and context-specific, so a utility function might not be appropriate.
While the functions are simple, if there are many such functions across the codebase, a utility function could improve maintainability. However, this specific instance does not provide enough evidence for such a need.
The comment should be deleted as it suggests an unnecessary refactor for a simple and context-specific function.
Workflow ID: wflow_F0b28CcNrIk6du86
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
Reviewed the code. it looks good. just a minor comment. please feel free to make the changes in another PR if merging this PR is urgent. I wasn't able to check the UI locally, as the ingestion keys API throws "Unauthorized" error when I visit Ingestion settings.
6f09478
to
b6baeca
Compare
https://signoz-team.slack.com/archives/C02C7P6KE06/p1731427069496209?thread_ts=1731409354.155829&cid=C02C7P6KE06
Important
Add toggle feature for enabling/disabling daily and per-second ingestion limits with UI and data structure updates.
MultiIngestionSettings.tsx
usingSwitch
component.LimitProps
,AddLimitProps
, andUpdateLimitProps
intypes.ts
to includeenabled
boolean forday
andsecond
limits.handleAddLimit
andhandleUpdateLimit
functions to handle the new toggle state.IngestionSettings.styles.scss
to accommodate new toggle layout.This description was created by for 6f09478. It will automatically update as commits are pushed.