-
Notifications
You must be signed in to change notification settings - Fork 328
chore: source category set to event stream as default #4226
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
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent update introduces a new constant Changes
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's AI:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
@coderabbitai review |
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.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- enterprise/reporting/event_stats.go (2 hunks)
- enterprise/reporting/event_stats_test.go (2 hunks)
Additional comments: 4
enterprise/reporting/event_stats.go (2)
- 29-38: The logic to default
sourceCategory
toEventStream
when it's empty is implemented correctly. Ensure that this default value is consistent with other parts of the codebase wheresourceCategory
might be used.
The search for
SourceCategory
in theenterprise/reporting/
directory shows that theSourceCategory
field is used in various places within the codebase. The references inreporting.go
andevent_stats_test.go
suggest that the field is indeed used elsewhere and that the logic for setting a default value might need to be consistent across these files.However, without more context on how
SourceCategory
is used in these other files, it's not possible to definitively say whether the defaulting logic inevent_stats.go
will cause any issues. The test cases inevent_stats_test.go
do include a case whereSourceCategory
is set to an empty string, which seems to be handled by the new logic.Given the information available, it seems that the change in
event_stats.go
is consistent with the intended behavior, but a full verification would require a deeper understanding of the codebase.Final review comment:
The update to default
sourceCategory
toEventStream
when empty is consistent with the changes described. However, ensure that this default value aligns with the usage ofsourceCategory
across the entire codebase, particularly inreporting.go
. It may be necessary to review the logic in related files to maintain consistency.
- 27-41: The changes to the
Record
method correctly implement the logic to preventsourceCategory
from being empty, aligning with the PR objective.enterprise/reporting/event_stats_test.go (2)
137-152: The new test case added to simulate the scenario where
SourceCategory
is empty and should default to "event stream" is correctly implemented.200-210: The assertion for the new metric with the default
EventStream
is correctly checking the expected behavior whenSourceCategory
is empty.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4226 +/- ##
==========================================
+ Coverage 73.88% 73.93% +0.04%
==========================================
Files 388 388
Lines 55098 55106 +8
==========================================
+ Hits 40709 40741 +32
+ Misses 12060 12043 -17
+ Partials 2329 2322 -7 ☔ View full report in Codecov by Sentry. |
Description
We have observed the source category coming to be as empty string in metrics and some customers are using prom exporter where the empty value label gets dropped. Reporting service assumes it as event stream when source cateogry is emtpy.
Linear Ticket
https://linear.app/rudderstack/issue/OBS-264/add-event-stream-in-source-category-as-default-value
Security
Summary by CodeRabbit
New Features
Bug Fixes
Record
method to address scenarios with missing source categories in event metrics.Tests