feat: remove unnecessary fields from UT payload #5422
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5422 +/- ##
==========================================
+ Coverage 74.77% 74.80% +0.03%
==========================================
Files 440 440
Lines 61648 61668 +20
==========================================
+ Hits 46095 46130 +35
+ Misses 13008 13000 -8
+ Partials 2545 2538 -7 ☔ View full report in Codecov by Sentry. |
cisse21
force-pushed
the
feat.optimiseUTPayload
branch
from
7e80c9c to
4250948
Compare
processor/transformer/transformer.go
Outdated
| return trans.transform(ctx, clientEvents, trans.userTransformURL(), batchSize, userTransformerStage) | ||
| var dehydratedClientEvents []TransformerEvent | ||
| for _, clientEvent := range clientEvents { | ||
| dehydratedClientEvent := clientEvent.Dehydrate() |
There was a problem hiding this comment.
What is a dehydrated client in this context? 🤔
There was a problem hiding this comment.
Added a little more in the description. Basically we send a common payload across user transformation , destination transformation and tracking plan which heavily inflates the payload size for each of them. The idea is to reduce the payload size by removing the unnecessary fields
There was a problem hiding this comment.
Whoever comes across this code will still have troubles understanding what Dehydrate means though, right? I don't think they'll go chasing the PR where you created this to get more context.
I suggest adding a comment here and also renaming the Dehydrate method to better reflect what it does:
- PruneFields
- StripUnnecessaryFields
- GetVersionsOnly
- ...
|
@cisse21 can you explain what's the screenshot in the description |
| @@ -270,7 +285,13 @@ func (trans *handle) Transform(ctx context.Context, clientEvents []TransformerEv | |||
|
|
|||
| // UserTransform function is used to invoke user transformer API | |||
| func (trans *handle) UserTransform(ctx context.Context, clientEvents []TransformerEvent, batchSize int) Response { | |||
There was a problem hiding this comment.
I was kind hoping for an approach where we introduce a separate client for user transformation.
As the requirements for dt and ut further diverge, keeping them under the same implementation only adds complexity.
processor/transformer/transformer.go
Outdated
| @@ -101,6 +101,21 @@ type TransformerEvent struct { | |||
| Credentials []Credential `json:"credentials"` | |||
| } | |||
|
|
|||
| func (e *TransformerEvent) Dehydrate() *TransformerEvent { | |||
There was a problem hiding this comment.
| func (e *TransformerEvent) Dehydrate() *TransformerEvent { | |
| func (e *TransformerEvent) Dehydrate() *HydradedTransformerEvent { |
Instead of using the same struct, with some fields missing, we can introduce new structs with only the fields we want to include.
This will make it easier to understand, what we plan to send and what not. Plus making the object allocation a bit lighter
There was a problem hiding this comment.
This would make it a bit difficult downstream as the transform function expected TransformerEvent. Was planning to do the same in the refactor to reduce the fields further
cisse21
force-pushed
the
feat.optimiseUTPayload
branch
from
dea0812 to
2b7a407
Compare
Description
This pull request aims to enhance the efficiency of the User Transformation (UT) payload by eliminating unnecessary fields, thereby improving performance by reducing payload size.
Changes Introduced:
Field Removal: Unused or redundant fields have been identified and removed from the UT payload structure.
In the image shown above the blue line represent the latency for a User Transformation with master and the yellow line represent the latency with the changes made in this PR. This change leads to 10-15% reduction in latency
Linear Ticket
Fixes PIPE-1845
Security