-
Notifications
You must be signed in to change notification settings - Fork 283
Support for aligning Slack message grouping intervals #1473
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: master
Are you sure you want to change the base?
Support for aligning Slack message grouping intervals #1473
Conversation
2936b00 to
d0856f1
Compare
ea1839d to
40729f9
Compare
40729f9 to
53c880a
Compare
| if values is None: | ||
| return {"summary": SummaryNotificationModeParams()} | ||
| def validate_interval_alignment(cls, values: Dict): | ||
| if values["aligned"]: |
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.
what if values is none? wont this throw? i would handle that case first
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.
It's marked as required, so the only possible values should be True and False. If somehow it's not set (should not be possible but can be forced), I think it should indeed throw an exception. If somehow it's set to None (again, should not be possible..) it will be evaluated to False, which I think is sensible here.
| def validate_interval_alignment(cls, values: Dict): | ||
| if values["aligned"]: | ||
| if values["interval"] < 24 * 3600: | ||
| if (24 * 3600) % values["interval"]: |
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.
not sure i understand, we allow the interval to be seconds but we dont support seconds, shouln't we change it to minutes or have it round up to the nearest minute instead of failing ? (inorder to support backward compatibility)
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.
There will be no backwards compatibility problems, as prior users don't use the interval aligning mechanism (it's not been released yet).
Also, we support aligning on thresholds shorter than a minutes here without problem. For example, if the user specs interval as 15 seconds, the intervals will be automatically aligned at x:00, x:15, x:30, x:45 (for consecutive minutes x).
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.
got it
| idx = 1 if resolved else 0 | ||
| if now_ts - self.start_ts > interval or not self.summary_table: | ||
| # Expired or the first summary ever for this group_key, reset the data | ||
| if not self.end_ts or now_ts > self.end_ts: |
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.
what if not self.summary_table?
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.
If end_ts is not set it means that the whole object is not initialized, and so summary_table is also not set. I basically simplified the condition here.
|
Warning Rate limit exceeded@moshemorad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Robert Szefler 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. |
No description provided.