-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: remove Hours and Months from Logs TTL dropdown for s3 cold storage #9234
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
base: main
Are you sure you want to change the base?
Conversation
- Add isS3ColdStorage prop to Retention component - Modify filter logic to exclude Hours and Months when type is 'logs' and isS3ColdStorage is true - This standardizes TTL schema by removing Hours option for s3 cold storage
Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗 |
M s kulkarni seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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.
Important
Looks good to me! 👍
Reviewed everything up to b7040e2 in 2 minutes and 14 seconds. Click for details.
- Reviewed
30
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/container/GeneralSettings/Retention.tsx:35
- Draft comment:
Good addition of the 'isS3ColdStorage' prop with a default value. Please ensure that its purpose is documented wherever the Retention component is used. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that the purpose of the 'isS3ColdStorage' prop is documented. This falls under the rule of not asking the author to ensure something is done, which is not allowed. The comment does not provide a specific code suggestion or point out a specific issue with the code itself.
2. frontend/src/container/GeneralSettings/Retention.tsx:57
- Draft comment:
The filter condition in 'menuItems' correctly excludes both 'hr' and 'month' when 'type' is 'logs' and 'isS3ColdStorage' is true. For improved readability, consider adding parentheses around the condition. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the suggestion would slightly improve readability, operator precedence is clear here and the current code is not confusing. The comment is about code style rather than a significant issue. The rules state we should not make purely informative comments or obvious suggestions. The parentheses could make the code more immediately clear to junior developers. The current form requires knowledge of operator precedence. The operator precedence here is standard and well-known. The suggestion is too minor to warrant a PR comment. Delete the comment as it's a minor style suggestion that doesn't meaningfully improve code quality or fix any real issues.
3. frontend/src/container/GeneralSettings/Retention.tsx:57
- Draft comment:
If a time unit like 'hr' or 'month' was previously selected, toggling to s3 cold storage (which filters these options out) might leave the component with an invalid selection. Consider adding logic to validate or reset 'selectedTimeUnit' when 'isS3ColdStorage' is true. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/container/GeneralSettings/Retention.tsx:58
- Draft comment:
There is a minor typographical issue: In the ternary operator condition, there's no space betweenTimeUnitsValues.month
and the colon. It might be clearer as: type === 'logs' && isS3ColdStorage ? option.value !== TimeUnitsValues.hr && option.value !== TimeUnitsValues.month : true, Consider adding a space before the colon for readability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the missing space, this is an extremely minor formatting issue that doesn't affect functionality. Most teams use automated formatters like Prettier that would handle this automatically. The comment doesn't add significant value and could be considered noise in the PR review process. The spacing inconsistency could potentially indicate that automated formatting isn't being used consistently in the project. Maybe this is worth flagging to ensure consistent code style? Even if formatting isn't automated, this is too minor an issue to warrant a PR comment. If formatting is a concern, it should be addressed through team-wide tooling or guidelines rather than individual PR comments. Delete the comment as it focuses on an extremely minor formatting issue that doesn't impact code quality or functionality.
Workflow ID: wflow_24IEfWQUoxCorFdW
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
📄 Summary
Removes Hours and Months options from Logs TTL dropdown when configuring s3 cold storage, leaving only Days as an available option. This standardizes the TTL schema for s3 cold storage as requested in the issue.
✅ Changes
🏷️ Required: Add Relevant Labels
ex:
frontend
bug
enhancement
ui
👥 Reviewers
🔍 Related Issues
Fixes #9190
Important
Removes
Hours
andMonths
from Logs TTL dropdown inRetention
component for S3 cold storage, leaving onlyDays
.Hours
andMonths
options from Logs TTL dropdown inRetention
component whenisS3ColdStorage
istrue
.Days
option remains for S3 cold storage.isS3ColdStorage
prop toRetention
component.Retention
to excludeHours
andMonths
whentype === 'logs'
andisS3ColdStorage === true
.This description was created by
for b7040e2. You can customize this summary. It will automatically update as commits are pushed.