feat: onboard new destination bing_ads_offline_conversion destination #4674
Conversation
|
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4674 +/- ##
==========================================
- Coverage 74.47% 74.45% -0.03%
==========================================
Files 413 416 +3
Lines 48752 49261 +509
==========================================
+ Hits 36310 36676 +366
- Misses 10056 10136 +80
- Partials 2386 2449 +63 ☔ View full report in Codecov by Sentry. |
anantjain45823
force-pushed
the
feat.onboard.bingads.offline.conversions
branch
from
4062615
to
462ceef
Compare
router/batchrouter/asyncdestinationmanager/bing-ads/offline-conversions/bulk_uploader.go
Outdated
Show resolved
Hide resolved
router/batchrouter/asyncdestinationmanager/bing-ads/audience/token.go
Outdated
Show resolved
Hide resolved
| @@ -1,4 +1,4 @@ | |||
| package bingads | |||
| package bingads_audience | |||
There was a problem hiding this comment.
| package bingads_audience | |
| package audience |
Per convention is ok to name this just audience, name conflicts can be solved on the package that imports it.
| fmt.Println("Error during unmarshalling event:", err) | ||
| return payload, err |
There was a problem hiding this comment.
| fmt.Println("Error during unmarshalling event:", err) | |
| return payload, err | |
| return payload, fmt.Errorf("unmarshalling event %w:", err) |
It is usually redundant to log the error and return it as well. It is better to be logged once with as much context as possible. Logging twice makes sense if it is impossible to transfer all the context.
Also, fmt.Println should not be used in production. logger should be used.
| fmt.Println("Error during unmarshalling event.fields:", err) | ||
| return payload, err |
There was a problem hiding this comment.
| fmt.Println("Error during unmarshalling event.fields:", err) | |
| return payload, err | |
| return payload, fmt.Errorf("unmarshalling event.fields: %w", err) |
| if event.Action != "insert" { | ||
| // validate for adjusted time | ||
| err := validateField(fields, "conversionAdjustedTime") | ||
| if err != nil { | ||
| return payload, err | ||
| } | ||
| if event.Action == "update" { | ||
| // validate for Adjustment Value | ||
| err := validateField(fields, "conversionValue") | ||
| if err != nil { | ||
| return payload, err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| if event.Action != "insert" { | |
| // validate for adjusted time | |
| err := validateField(fields, "conversionAdjustedTime") | |
| if err != nil { | |
| return payload, err | |
| } | |
| if event.Action == "update" { | |
| // validate for Adjustment Value | |
| err := validateField(fields, "conversionValue") | |
| if err != nil { | |
| return payload, err | |
| } | |
| } | |
| } | |
| if event.Action != "insert" { | |
| // validate for adjusted time | |
| err := validateField(fields, "conversionAdjustedTime") | |
| if err != nil { | |
| return payload, err | |
| } | |
| } | |
| if event.Action == "update" { | |
| // validate for Adjustment Value | |
| err := validateField(fields, "conversionValue") | |
| if err != nil { | |
| return payload, err | |
| } | |
| } |
[minor] reducing nesting to improve reliability
There was a problem hiding this comment.
Incorporated
| data := map[string]interface{}{ | ||
| "message": map[string]interface{}{ | ||
| "fields": event.Fields, | ||
| "Action": event.Action, | ||
| }, | ||
| "metadata": map[string]interface{}{ | ||
| "jobId": job.JobID, | ||
| }, |
There was a problem hiding this comment.
Can we use a struct instead? Improve readability and better performance.
| data := map[string]interface{}{ | |
| "message": map[string]interface{}{ | |
| "fields": event.Fields, | |
| "Action": event.Action, | |
| }, | |
| "metadata": map[string]interface{}{ | |
| "jobId": job.JobID, | |
| }, | |
| data := MappedData{ | |
| MappedDataMessage{ | |
| Fields: event.Fields, | |
| Action: event.Action, | |
| .... |
Note: more appropriate naming can be used.
There was a problem hiding this comment.
Using struct now
| importIds = append(importIds, uploadBulkFileResp.RequestId) | ||
| failedJobs = append(failedJobs, actionFile.FailedJobIDs...) | ||
| successJobs = append(successJobs, actionFile.SuccessfulJobIDs...) | ||
| // requestIdToJobIdMap[uploadBulkFileResp.RequestId] = successJobs |
There was a problem hiding this comment.
| // requestIdToJobIdMap[uploadBulkFileResp.RequestId] = successJobs |
Is this needed ?
| if err != nil { | ||
| b.logger.Error("Errored in Marshalling parameters" + err.Error()) | ||
| } | ||
| allErrors := router_utils.EnhanceJSON([]byte(`{}`), "error", strings.Join(errors, commaSeparator)) |
There was a problem hiding this comment.
why not use proper JSON marshalling?
There was a problem hiding this comment.
Updated it to use proper JSON marshalling
| for _, actionFile := range actionFiles { | ||
| err = os.Remove(actionFile.ZipFilePath) | ||
| if err != nil { | ||
| b.logger.Error("Error in removing zip file: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe this can be moved in defer function, earlier in the code. In case we need to return earlier without deleting the files
There was a problem hiding this comment.
These are temporary files and needs to be deleted every time upload is finished.
If we have any error while creating the zip files we return else once created we return only after deleting the files
| if err != nil { | ||
| fmt.Printf("Failed to create temporary directory: %v\n", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
If any of the if err != nil fails it would be hard to debug, since you print and just return, the test suite would not register any failure and silently return. Without verbose mode, logs are not going be shown as well.
Since you are using gomega, the correct way to handle errors is:
| if err != nil { | |
| fmt.Printf("Failed to create temporary directory: %v\n", err) | |
| return | |
| } | |
| Exepect(err).ShouldNot(HaveOccurred(), "creating temporary directory") |
https://onsi.github.io/gomega/#haveoccurred
Similar of all the if err != nil in the code.
There was a problem hiding this comment.
Changed all err handling in test cases with expect(err).ShouldNot(HaveOccurred(),'some_text')
Description
Onboarding a new batch router bulk upload O-Auth destination Bing Ads Offline Conversions.
-> using transform utility to validate the incoming payload with destination specific required parameters
-> Keeping the batch size as 1000 as a single CSV can at most contain 100 batch records.
-> We have three actions for this integrations
insert,updateanddeleteand for each of them we create a file separately if the batch of 1000 has atleast a single specific action-> The procedure for bulk API is as follows:
1. Transform/Validate the payload ( having necessary keys or not ) and create a csv file based on action type and add records to that file
2. Get an upload url : URL where the file will be uploaded
3. Upload file: Upload the files from step 1
4. Poll: Fetch file status if failed with errors or with success
If failed with errors we dive into the result file given in the poll response and check which jobId's that got failed
and which one got passed.
Linear Ticket
https://linear.app/rudderstack/issue/INT-1922/rudder-server-changes-in-async-destination-to-deliver-events
Security