-
Notifications
You must be signed in to change notification settings - Fork 312
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
fix(warehouse): added support for filtering on the uploads and calculating aborted events for task_run_id #2975
Conversation
warehouse/warehouse.go
Outdated
if sourceID == "" || taskRunID == "" { | ||
pkgLogger.Errorf("[WH]: pending-events: Empty source_id or task_run_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.
I'll make sure we have task_run_id coming always in pending events api
|
||
var count int64 | ||
if err := uploads.db.QueryRowContext(ctx, query, args...).Scan(&count); err != nil { | ||
return 0, fmt.Errorf("scanning count into local variable: %w", err) |
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.
return 0, fmt.Errorf("scanning count into local variable: %w", err) | |
return 0, fmt.Errorf("count uploads: %w", err) |
The error seems to be misleading w.r.t. local variable. Can we update it?
warehouse/warehouse.go
Outdated
if sourceID == "" { | ||
pkgLogger.Errorf("[WH]: pending-events: Empty source id") | ||
if sourceID == "" || taskRunID == "" { | ||
pkgLogger.Errorf("[WH]: pending-events: Empty source_id or task_run_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.
Can we make it a structured log? Also, should we make this warning?
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.
Why a warning and not error ?
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.
Since the error is not causing our application to be in trouble or involving us in the investigation immediately. Also, since these APIs are being polled continuously by the extract, we might see too much noise as well.
I thought it was okay to mark them as a warning.
warehouse/warehouse.go
Outdated
|
||
if err != nil { | ||
err := fmt.Errorf("getting pending uploads count: %v", err) | ||
pkgLogger.Errorf("[WH]: %v", err) |
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.
Can we make it a structured log? Also, should we make this warning?
warehouse/warehouse.go
Outdated
if err != nil { | ||
err := fmt.Errorf("error getting pending uploads : %v", err) | ||
err := fmt.Errorf("getting aborted uploads count : %v", err) | ||
pkgLogger.Errorf("[WH]: %v", err) |
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.
Can we make it a structured log? Also, should we make this warning?
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2975 +/- ##
==========================================
- Coverage 53.62% 52.93% -0.69%
==========================================
Files 347 334 -13
Lines 53806 51879 -1927
==========================================
- Hits 28853 27464 -1389
+ Misses 23320 22808 -512
+ Partials 1633 1607 -26
... and 92 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
if pendingEventsReq.TaskRunID != "" { | ||
filterBy = append(filterBy, warehouseutils.FilterBy{Key: "metadata->>'source_task_run_id'", Value: pendingEventsReq.TaskRunID}) | ||
filters := []repo.FilterBy{ | ||
{Key: "source_id", Value: sourceID}, |
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.
Have we defined the table columns as constants? Maybe it would be safer to use constants.
b9f1b56
to
6e5e3c0
Compare
e04bd7d
to
757edf4
Compare
Description
The PR augments the capability of repo to count over wh_uploads using generic filters which gets used in the pending events API when calculating pending events and aborted events.
Notion Ticket
https://www.notion.so/rudderstacks/Fix-the-Pending-Events-API-to-return-always-if-the-events-have-been-aborted-in-a-boolean-flag-e251b60a20484513ad802fe4ed50d790
Security