feat: start sending impact metrics in payload #341
Conversation
| List<CollectedMetric> impactMetricsOrNull = | ||
| impactMetrics.isEmpty() ? null : impactMetrics; | ||
|
|
||
| ClientMetrics metrics = | ||
| new ClientMetrics(unleashConfig, bucket, impactMetricsOrNull); |
There was a problem hiding this comment.
There is design from node SDK, that if we do not have any metrics, we do not add the empty array to payload.
| } | ||
|
|
||
| @Test | ||
| public void should_serialize_histogram_with_positive_infinity_as_plus_inf_string() { |
There was a problem hiding this comment.
Moving this to http level, so we can catch also if serializer was not mounted.
| } | ||
|
|
||
| @Test | ||
| public void should_send_impact_metrics_with_histogram_and_plus_inf_bucket() |
There was a problem hiding this comment.
Following the java test style, we verify that +Inf is being sent.
| } | ||
|
|
||
| @Test | ||
| public void should_include_impact_metrics_in_clientmetrics_payload() throws YggdrasilError { |
There was a problem hiding this comment.
These tests are same as in node SDK.
| 300, | ||
| unleashConfig.getUnleashURLs().getClientMetricsURL()); | ||
| this.engine = engine; | ||
| ImpactMetricsDataSource configuredRegistry = unleashConfig.getImpactMetricsRegistry(); |
There was a problem hiding this comment.
why the registry lives in the config and is not passed as other dependencies via constructor?
There was a problem hiding this comment.
ImpactMetricsDataSource is configurable, so it belongs in UnleashConfig, and UnleashMetricServiceImpl reads it from there (with a default fallback), just like it does for other configurable dependencies.
| unleashConfig.getUnleashURLs().getClientMetricsURL()); | ||
| this.engine = engine; | ||
| ImpactMetricsDataSource configuredRegistry = unleashConfig.getImpactMetricsRegistry(); | ||
| this.impactMetricsRegistry = |
There was a problem hiding this comment.
this is a code smell (https://lab.abilian.com/Tech/Architecture%20%26%20Software%20Design/Dependency%20Inversion/DI%20anti-patterns/#bastard-injection-poor-mans-injection). as a user I don't know when I should pass the registry and when it will be created automatically.
There was a problem hiding this comment.
Good catch, fixed it by following what we do with UnleashScheduledExecutorImpl
Pull Request Test Coverage Report for Build 20338631979Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
3 participants
Start sending impact metrics in metrics payload.