Refactors the MetricsManager to improve reliability and monitoring control for metric delivery. #63
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xiajon
reviewed
Jun 4, 2025
| response.id?.let { id -> | ||
| updatePlaceholderMessage(oldId = recentlySentMessage.id, newId = id) | ||
| } | ||
| true | ||
| }.onFailure { exception -> | ||
| metricsManager.addCountMetric(MetricName.SendMessage) |
There was a problem hiding this comment.
How does this fix the message id issue? This code just moves the metric down and replaces runCatching with try
saipreetham16
force-pushed
the
sleburu/fix_CSM
branch
from
b44f9bb to
fc6f16c
Compare
saipreetham16
changed the title
saipreetham16
force-pushed
the
sleburu/fix_CSM
branch
from
fc6f16c to
b7ff131
Compare
saipreetham16
changed the title
…ntrol for metric delivery.
saipreetham16
force-pushed
the
sleburu/fix_CSM
branch
from
b7ff131 to
23abef9
Compare
xiajon
reviewed
Jun 11, 2025
| } | ||
|
|
||
| private fun addMetric(metric: Metric) { | ||
| if (_isCsmDisabled) { |
There was a problem hiding this comment.
nvm i see it is added above
xiajon
approved these changes
Jun 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number: 63
Description:
Refactors the MetricsManager to improve reliability and monitoring control for metric delivery.
Before:
shouldRetry.monitorAndSendMetrics()with limited lifecycle control.metricListwas reassigned (metricList = mutableListOf()) after a successful send, which could lead to potential race conditions.After:
retryCountandmaxRetries(set to 3).metricListqueue, ensuring they are retried in the next monitoring cycle.startMonitoring()andstopMonitoring().synchronized(metricList)blocks instead of reassigning the list to ensure thread-safe updates.initialDelay(1s) andmonitoringPeriod(10s) for better configurability and consistency.These changes enhance the delivery guarantees of metrics, reduce potential for data loss, and improve maintainability and concurrency handling.
Functional backward compatibility:
Does this change introduce backwards incompatible changes? [NO]
Does this change introduce any new dependency? [NO]
Testing:
Is the code unit tested?
NO
List manual testing steps:
Added new test metric and validated on CloudWatch metrics.