-
Notifications
You must be signed in to change notification settings - Fork 105
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
[OTE-880] Emit log in case of collisions #2500
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (1)
25-26
: LGTM: Error handling changed to use metrics.The modification from logging to using metrics for tracking username generation collisions is well-implemented and aligns with the PR objectives. The use of
config.SERVICE_NAME
in the metric name is a good practice for identifying the source.Consider adding minimal logging (e.g., at DEBUG level) in addition to the metric. This could help with debugging in case the collision rate becomes unexpectedly high:
stats.increment(`${config.SERVICE_NAME}.subaccount-username-generator.collision`, 1); logger.debug(`Username collision occurred for subaccountId: ${subaccount.subaccountId}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (2)
1-1
: LGTM: Import changes align with PR objectives.The changes in imports are consistent with the goal of transitioning from logging to metrics for username generation. The addition of
stats
andconfig
imports, along with the removal of thelogger
import, supports this transition effectively.Also applies to: 7-7
Line range hint
1-26
: Overall impact: Successful transition to metrics-based tracking.The changes effectively implement the transition from logging to metrics for username generation collisions, aligning well with the PR objectives. This modification will provide better visibility into collision frequencies over time.
To ensure completeness, please verify if any monitoring or alerting systems need to be updated to work with the new metric instead of log-based alerts. Run the following script to check for any log-based alerting configurations that might need updating:
If this script returns any results, consider updating those configurations to use the new metric:
${config.SERVICE_NAME}.subaccount-username-generator.collision
.✅ Verification successful
Overall verification: Metrics-based tracking implemented successfully.
No log-based alerting configurations related to username generation collisions were found. This confirms that the transition from logging to metrics for tracking collisions has been successfully completed, providing improved visibility into collision frequencies over time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for log-based alerting configurations related to username generation # Test: Search for alerting configurations mentioning username generation or collisions rg --type yaml --type json -i "username.*generation|collision" -g "*alert*.{yaml,yml,json}"Length of output: 242
Script:
#!/bin/bash # Updated script to search for log-based alerting configurations related to username generation collisions # Removed restrictive -g flag to ensure all relevant files are searched rg --type yaml --type json -i "username.*generation|collision"Length of output: 1855
Script:
#!/bin/bash # Updated script to search for log-based alerting configurations related to username generation collisions # Excluding lock files to avoid irrelevant matches rg --type yaml --type json -i "username.*generation|collision" --glob '!*.lock.{yaml,yml,json}'Length of output: 1888
d86ea7c
to
7d94c06
Compare
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit a18eb61)
Co-authored-by: Mohammed Affan <[email protected]>
Changelist
Collisions are expected and in case of collisions - emit a log.
Otherwise emit error log
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.Summary by CodeRabbit
Bug Fixes
Chores