Add feature flag for Kinesis lag metric#9807
Conversation
| @Test | ||
| public void testDiscoverExistingPublishingAndReadingTaskWithoutTimeLagMetrics() throws Exception | ||
| { | ||
| // Like testDiscoverExistingPublishingAndReadingTask, but with time lag metrics disabled |
There was a problem hiding this comment.
Perhaps refactor this into helper method that also used by testDiscoverExistingPublishingAndReadingTask, since it looks like the only differences are (1) whether time lag metrics are enabled/disabled in the supervisor spec and (2) the assert for the proper lag metric value in the active report.
There was a problem hiding this comment.
I refactored this into a helper, and added a check for the time lag metrics in the supervisor aggregate-level report
There was a problem hiding this comment.
There was another difference, whether supervisorRecordSupplier.getPartitionTimeLag is called or not
| } | ||
|
|
||
| @Test | ||
| public void testTimeLagMetricsDisabled() throws Exception |
There was a problem hiding this comment.
With regards to disabling the time lag metric, what behavior is tested by testDiscoverExistingPublishingAndReadingTaskWithoutTimeLagMetrics that is not covered by this test?
What do you think about refactoring some of the logic here so that it's reused by testNoInitialState?
There was a problem hiding this comment.
testDiscoverExistingPublishingAndReadingTaskWithoutTimeLagMetrics was checking for time lag metrics in the active tasks portion of the supervisor status report, which testNoInitialState didn't do.
testNoInitialState had checks for time lag metrics in the aggregate-level portion of the supervisor report which testDiscoverExistingPublishingAndReadingTaskWithoutTimeLagMetrics didn't
I ended up deleting testTimeLagMetricsDisabled and collapsed the time lag metrics tests into testDiscoverExistingPublishingAndReadingTask* test pair which now checks all 3 areas.
|
#9819 might make this not necessary, but even still it feels lopsided to me that this setting only can only provide control over time lag and not both time and record count lag. Thoughts? |
|
I'll close this for now as I agree with #9819 making this not needed |
4 participants
This PR adds a new
enableTimeLagMetricsproperty to the Kinesis supervisor tuningConfig that controls whether themillisBehindLatest-based metrics are emitted, and also whether such information is included in the supervisor status report.This PR has: