Add aggregate metrics for ddb source export and stream #3724
Conversation
Signed-off-by: Taylor Gray <[email protected]>
graytaylor0
requested review from
chenqi0805,
engechas,
dinujoh,
kkondaka,
asifsmohammed,
dlvenable and
oeyh
as code owners
| DescribeExportResponse resp = dynamoDBClient.describeExport(request); | ||
| manifestKey = resp.exportDescription().exportManifest(); | ||
|
|
||
| } catch (final InternalServerErrorException e) { | ||
| dynamoAggregateMetrics.getExport5xxErrors().increment(); | ||
| LOG.error("Unable to get manifest file for export " + exportArn); |
There was a problem hiding this comment.
Should we log the error here too? Same applies for the catch section below
There was a problem hiding this comment.
Ah yeah we should log that
| } catch(final InternalServerErrorException e) { | ||
| LOG.error("Received an internal server exception from DynamoDB while listing shards: {}", e.getMessage()); | ||
| dynamoDBSourceAggregateMetrics.getStream5xxErrors().increment(); | ||
| return shards; |
There was a problem hiding this comment.
Should we rethrow the error here instead of potentially returning partial shards?
There was a problem hiding this comment.
I don't think there's anything wrong with returning partial shards, it just means we will only check those shards for child shards. The next time we run describeStream we will still go through the same shards again and create child shards for them (even the ones we return here) until 24 hours is up (when the shard would expire).
Signed-off-by: Taylor Gray <[email protected]>
| ExportTableToPointInTimeResponse response = dynamoDBClient.exportTableToPointInTime(req); | ||
|
|
||
| String exportArn = response.exportDescription().exportArn(); | ||
| String status = response.exportDescription().exportStatusAsString(); | ||
| LOG.debug("Export Job submitted with ARN {} and status {}", exportArn, status); | ||
| return exportArn; | ||
| } catch (final InternalServerErrorException e) { | ||
| dynamoAggregateMetrics.getExport5xxErrors().increment(); |
There was a problem hiding this comment.
should we also consider adding pipeline specific metric ?
There was a problem hiding this comment.
I don't think there's any value in that for these metrics at least. There may be other pipeline specific metrics we can add, but that can be a future PR
Signed-off-by: Taylor Gray <[email protected]> (cherry picked from commit 1af1ce9)
Signed-off-by: Taylor Gray <[email protected]> (cherry picked from commit 1af1ce9) Co-authored-by: Taylor Gray <[email protected]>
Description
This change adds the following aggregate metrics (metrics without a
sub-pipelineprefix) to dynamoDB source pipelinesstream5xxErrors- The number of errors encountered when calling stream APIs (getRecords,getShardIterator)streamApiInvocations- The number of invocations of stream APIsexport5xxErrors- The number of errors encountered when calling export APIs (exportTableToPointInTime,describeExport)exportApiInvocations- The number of invocations of export APIsCheck 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.