-
Notifications
You must be signed in to change notification settings - Fork 692
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2455 +/- ##
============================================
- Coverage 81.29% 74.05% -7.25%
+ Complexity 2343 2119 -224
============================================
Files 264 264
Lines 7663 7663
Branches 790 790
============================================
- Hits 6230 5675 -555
- Misses 1097 1620 +523
- Partials 336 368 +32
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddumelendez Thanks for another awesome contribution to the project!
Complete with an integration test!
It looks good to me, but I'll let @elefeint do a more detailed review since she's focusing on the metrics support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eddu, thank you -- the sample is great. I have a concern about the test, though -- see in comments.
...spring-cloud-gcp-metrics-sample/src/test/java/com/example/MetricsSampleApplicationTests.java
Outdated
Show resolved
Hide resolved
...spring-cloud-gcp-metrics-sample/src/test/java/com/example/MetricsSampleApplicationTests.java
Show resolved
Hide resolved
...cloud-gcp-samples/spring-cloud-gcp-metrics-sample/src/main/java/com/example/Application.java
Outdated
Show resolved
Hide resolved
...gcp-samples/spring-cloud-gcp-metrics-sample/src/main/java/com/example/ExampleController.java
Outdated
Show resolved
Hide resolved
...spring-cloud-gcp-metrics-sample/src/test/java/com/example/MetricsSampleApplicationTests.java
Show resolved
Hide resolved
@elefeint thanks for the feedback! really good points. I have made the changes so test can reproduce every time without trusting in previous executions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay -- there are a couple small issues that cropped up, otherwise looks good.
...spring-cloud-gcp-metrics-sample/src/test/java/com/example/MetricsSampleApplicationTests.java
Outdated
Show resolved
Hide resolved
...spring-cloud-gcp-metrics-sample/src/test/java/com/example/MetricsSampleApplicationTests.java
Outdated
Show resolved
Hide resolved
@elefeint PR updated. I got it wrong the previous time. Now, the test will hit just once the endpoint and will look for the metrics every 5 seconds in a lifetime of 4 minutes. Thanks for the feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the help!
See gh-2403