fix(tests): Re-enable Metrics integration test by renaming #118
Conversation
|
We are - which is why this test results wasn't firing. I was getting a permission denied error in the old repo w GHA and investigated why I wasn't getting it in the new repo. The 'metrics' module is already included in the big list of parallelized integration tests. |
|
|
||
| === Stackdriver Metrics Explicitly Enabled | ||
|
|
||
| If you're importing the `spring-cloud-gcp-starter-metrics` module, the Stackdriver metrics now need to be explicitly enabled with the property `management.metrics.export.stackdriver.enabled=true` |
There was a problem hiding this comment.
Hmm. Ideally, our starter would provide all the necessary dependencies, so the end user does not need to specify an extra property. Would that be possible?
There was a problem hiding this comment.
I think this property is checked for in Spring Boot, so I don't think we can enable it for the user. Source: https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/export/stackdriver/StackdriverMetricsExportAutoConfiguration.java#L50
There was a problem hiding this comment.
The fault should be enabled, but it looks like something is setting management.metrics.export.defaults.enabled=false. Looking more into it.
There was a problem hiding this comment.
It's done here: spring-projects/spring-boot@b1830da#diff-196296cded479f093a123da98914f159381d3eef89c33f20148075a58085b34fR53
Basically, Spring Boot Test automatically disabled metrics export, unless you add the @AutoConfigureMetrics annotation.
I'll update this branch with that.
There was a problem hiding this comment.
It's ignored in tests, so we need to move the property from main/resources to test/resources, and no need to document for end users.
|
Tests are back to green now. |
| @@ -28,6 +28,10 @@ | |||
| </dependencyManagement> | |||
|
|
|||
| <dependencies> | |||
| <dependency> | |||
| <groupId>org.springframework.boot</groupId> | |||
| <artifactId>spring-boot-starter-actuator</artifactId> | |||
There was a problem hiding this comment.
@ttomsu When running from IntelliJ, removing it didn't break the sample for me, but now I see that it's actually required. In fact, I think it needs to be moved into our spring-cloud-gcp-starter-metrics.
|
I moved all changes unrelated to re-enabling the test to a separate PR. See: #121 |
ttomsu
dismissed stale reviews from meltsufin and elefeint
Collaborated on change.
…udPlatform#118) * fix(tests): Re-enable Metrics integration test by renaming
Looks like metrics wasn't working this whole time because this test never got picked up. Whoops...