feat: onboard new custom destination: wunderkind #4798
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 Configuration File (
|
|
This PR is considered to be stale. It has been open 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4798 +/- ##
==========================================
+ Coverage 74.42% 74.45% +0.02%
==========================================
Files 427 428 +1
Lines 49658 49727 +69
==========================================
+ Hits 36959 37022 +63
+ Misses 10271 10259 -12
- Partials 2428 2446 +18 ☔ View full report in Codecov by Sentry. |
|
|
||
| func init() { | ||
| pkgLogger = logger.NewLogger().Child("streammanager").Child(lambda.ServiceName) | ||
| } |
There was a problem hiding this comment.
Can we avoid the use init and the global here?
Is it possible to inject logger into the struct?
| statusCode := 400 | ||
| respStatus := "Failure" | ||
| responseMessage := string(response.Payload) | ||
| pkgLogger.Errorf("[Wunderkind] Function execution error :: %d : %s : %s", statusCode, respStatus, responseMessage) |
There was a problem hiding this comment.
Prefix should not be needed since it was added using Child
| } | ||
|
|
||
| var invokeInput lambda.InvokeInput | ||
| wunderKindLambda := config.GetEnv(WUNDERKIND_LAMBDA) |
There was a problem hiding this comment.
Can you provide some more clarification on this?
There was a problem hiding this comment.
You would need to take config as a parameter in the constructor and store it as part of Producer struct. Later you can us it producer.conf.GetString(WunderkindLambda, "")
// NewProducer creates a producer based on destination config
func NewProducer(conf *config.Config) (*Producer, error) {
awsSession, err := awsutil.CreateSession(&awsutil.SessionConfig{
Region: conf.GetString(WunderkindRegion, ""),
IAMRoleARN: conf.GetString(WunderkindIamRoleArn, ""),
ExternalID: conf.GetString(WunderkindExternalId, ""),
RoleBasedAuth: true,
})
if err != nil {
return nil, fmt.Errorf("creating session: %w", err)
}
return &Producer{
conf: conf,
logger: logger.NewLogger().Child("streammanager").Child("wunderkind"),
client: lambda.New(awsSession),
}, nil
}There was a problem hiding this comment.
addressed.
I have used config.default in caller function of NewProducer
| ctrl := gomock.NewController(t) | ||
| mockClient := mock_lambda.NewMockLambdaClient(ctrl) | ||
| producer := &WunderkindProducer{client: mockClient} | ||
| mockLogger := mock_logger.NewMockLogger(ctrl) |
There was a problem hiding this comment.
Use mem logger instead, much more maintainable.
There was a problem hiding this comment.
Are you referring about this?
https://pkg.go.dev/go.chromium.org/luci/common/logging/memlogger
There was a problem hiding this comment.
@lvrach We don't have mem logger implementation. But in this particular case, we can simply asssert for returned variables, Don't see much of a value in asserting if we are logging or not?
| ctrl := gomock.NewController(t) | ||
| mockClient := mock_lambda.NewMockLambdaClient(ctrl) | ||
| producer := &WunderkindProducer{client: mockClient} | ||
| mockLogger := mock_logger.NewMockLogger(ctrl) |
There was a problem hiding this comment.
@lvrach We don't have mem logger implementation. But in this particular case, we can simply asssert for returned variables, Don't see much of a value in asserting if we are logging or not?
Description
Onboard new custom destination wunderkind
Linear Ticket
Resolves INT-2276
https://linear.app/rudderstack/issue/INT-2276/creating-custom-destination-in-rudder-server
Security