Add DLQ support to CloudWatchLogs sink #5542
Conversation
kkondaka
requested review from
sb2k16,
chenqi0805,
engechas,
san81,
srikanthjg,
graytaylor0,
dinujoh,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
| private final CloudWatchLogsDispatcher cloudWatchLogsDispatcher; | ||
| private final Buffer buffer; | ||
| private final CloudWatchLogsLimits cloudWatchLogsLimits; | ||
| private List<EventHandle> bufferedEventHandles; | ||
| //private List<EventHandle> bufferedEventHandles; |
There was a problem hiding this comment.
Some comments that can be removed in this file
| if (cloudWatchLogsSinkConfig.getDlq() != null) { | ||
| String region = awsConfig.getAwsRegion().toString(); | ||
| String role = awsConfig.getAwsStsRoleArn(); | ||
| dlqPushHandler = new DlqPushHandler(pluginFactory, pluginSetting, pluginMetrics, cloudWatchLogsSinkConfig.getDlq(), region, role, "cloudWatchLogs"); | ||
| } |
There was a problem hiding this comment.
Can we use AwsCredentialsSupplier default region and role when this is not specified in the DLQ config? We shold also make sure this DLQ supports the sts header overrides
There was a problem hiding this comment.
I am just following what we do for opensearch sink dlq. There is no point in doing much different because pipeline DLQ will make replace this.
There was a problem hiding this comment.
What's the difference between using awsCredentialsSupplier.getDefaultRegion() vs awsConfig.getAwsRegion()?
|
|
||
| public DlqPushHandler(final PluginFactory pluginFactory, final PluginSetting pluginSetting, final PluginMetrics pluginMetrics, | ||
| final PluginModel dlqConfig, final String region, final String role, final String metricsPrefix) { | ||
| Map<String, Object> dlqSettings = dlqConfig.getPluginSettings(); |
There was a problem hiding this comment.
What about the sts_header_overrides
There was a problem hiding this comment.
I am just following what we do for opensearch sink dlq. There is no point in doing much different because pipeline DLQ will make replace this.
There was a problem hiding this comment.
But you aren't doing it exactly the same. In OpenSearch sink we pass the full dlq config here (
), which includes sts header overridesThere was a problem hiding this comment.
@graytaylor0 Yes. That's there in data-prepper-plugins/failures-common/src/main/java/org/opensearch/dataprepper/plugins/dlq/s3/ The config is under s3: not under dlq: It is already supported
There was a problem hiding this comment.
So if users pass
dlq:
s3:
sts_header_overrides:
....
to cloudwatch sink those will get applied?
| @@ -27,11 +28,14 @@ private CloudWatchLogsClientFactory() { | |||
| * @return CloudWatchLogsClient used to interact with CloudWatch Logs services. | |||
| */ | |||
| public static CloudWatchLogsClient createCwlClient(final AwsConfig awsConfig, final AwsCredentialsSupplier awsCredentialsSupplier) { | |||
| final AwsCredentialsOptions awsCredentialsOptions = convertToCredentialOptions(awsConfig); | |||
| final AwsCredentialsProvider awsCredentialsProvider = awsCredentialsSupplier.getProvider(awsCredentialsOptions); | |||
| final AwsCredentialsProvider awsCredentialsProvider = awsConfig != null ? awsCredentialsSupplier.getProvider(convertToCredentialOptions(awsConfig)) : awsCredentialsSupplier.getProvider(AwsCredentialsOptions.defaultOptionsWithDefaultCredentialsProvider()); | |||
There was a problem hiding this comment.
This is not quite write. All you need to do here is to pass an empty AwsCredentialsOptions to this method here. Using defaultOptionsWithDefaultCredentialProvider() will use the default credentials of the data prepper node, not the default role configured
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
Description
Add DLQ support to CloudWatchLogs sink
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.