-
Notifications
You must be signed in to change notification settings - Fork 305
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
chore: add stage to transformer request time metric #4848
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 Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 #4848 +/- ##
==========================================
+ Coverage 74.28% 74.46% +0.17%
==========================================
Files 418 418
Lines 49092 48998 -94
==========================================
+ Hits 36469 36486 +17
+ Misses 10239 10135 -104
+ Partials 2384 2377 -7 ☔ View full report in Codecov by Sentry. |
d9fe0ca
to
6736ba7
Compare
efdfe4a
to
82f2bc5
Compare
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.
LGTM. added a minor comment. fasthttp client didn't help and hence we're removing it?
destType := clientEvents[0].Destination.DestinationDefinition.Name | ||
transformURL := trans.destTransformURL(destType) | ||
return trans.transform(ctx, clientEvents, transformURL, batchSize, destTransformerStage) | ||
return trans.transform(ctx, clientEvents, trans.destTransformURL(clientEvents[0].Destination.DestinationDefinition.Name), batchSize, destTransformerStage) |
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.
Shouldn't it be safe to check the length? even if the current caller ensures a empty slice is not sent?
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.
We have a check in the caller here
We can be safe but we should be safe even in UserTransform
and Validate
too as they are similar. Felt removing is the best way forward
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.
Just looking at this function: It is just doing a pass through. Couldn't we just get rid of this and rename trans.transform to trans.Transform?
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.
@sivashanmukh will take it up in a new PR. Will merge this PR for now
We had a CFD after enabling |
82f2bc5
to
b10c0f9
Compare
processor/transformer/transformer.go
Outdated
} | ||
bodyBytes, err := io.ReadAll(resp.Body) | ||
if err != nil { | ||
fmt.Println("Unable to read response into bytes with error : ", err.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.
Should we use a logger here?
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 wanted to get rid of this function too if we were okay with it
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 didn't see transformer_build_version set in either of user_transformer or dest_transformer pods enviornments in multi-tenant rudder-transformer and user-transformer. Not sure if its being set from elsewhere.
Can potentially get rid of this altogether 👍
b10c0f9
to
dced7ce
Compare
dced7ce
to
93b23fa
Compare
Description
Linear Ticket
Fixes PIPE-1243
Security