Enable/Disable Endpoint timestamps #254

MatthewBerk · 2021-07-29T04:58:44Z

What:
Added timestamp to response message when enable/disable chaos monkey with the endpoints /chaosmonkey/enable and /chaosmonkey/disable.
Modified test cases for enableChaosMonkey() and disableChaosMonkey() methods in order to handle timestamp now being present.

Why:
#246 (comment)

How:
Using java.time.Clock to get current time of user and attaching it to the already present "Chaos Monkey is enabled/disabled" message.

Checklist:

Documentation added N/A
Changelog updated
Tests added N/A
Ready to be merged

codecov-commenter · 2021-07-29T18:11:27Z

Codecov Report

Merging #254 (1245160) into main (0830691) will decrease coverage by 0.72%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##               main     #254      +/-   ##
============================================
- Coverage     76.30%   75.57%   -0.73%     
- Complexity      380      381       +1     
============================================
  Files            78       80       +2     
  Lines          1034     1040       +6     
  Branches         66       66              
============================================
- Hits            789      786       -3     
- Misses          211      217       +6     
- Partials         34       37       +3

Impacted Files	Coverage Δ
...s/monkey/endpoints/dto/ChaosMonkeyDisabledDto.java	`66.66% <66.66%> (ø)`
...os/monkey/endpoints/dto/ChaosMonkeyEnabledDto.java	`66.66% <66.66%> (ø)`
...chaos/monkey/endpoints/ChaosMonkeyJmxEndpoint.java	`84.61% <100.00%> (ø)`
...haos/monkey/endpoints/ChaosMonkeyRestEndpoint.java	`91.30% <100.00%> (ø)`
...ring/boot/chaos/monkey/assaults/MemoryAssault.java	`83.58% <0.00%> (-10.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0830691...1245160. Read the comment docs.

WtfJoke

Hey @MatthewBerk

I left feedback in the comments. Left some ideas for improving the code.
Let me know if something is unclear

WtfJoke · 2021-07-30T08:43:23Z

chaos-monkey-docs/src/main/asciidoc/changes.adoc

@@ -4,13 +4,15 @@
 Built with Spring Boot {spring-boot-version}

 === Bug Fixes
+- https://github.com/codecentric/chaos-monkey-spring-boot/pull/252[#252] Update Maven Wrapper from 0.5.5 to 0.5.6.


These changes already have been merged.

You need to rebase latest changes of master or merge master into your branch.

Will do. Had pushed the day before that change got merged!

WtfJoke · 2021-07-30T08:44:11Z

chaos-monkey-docs/src/main/asciidoc/changes.adoc

 - https://github.com/codecentric/chaos-monkey-spring-boot/pull/251[#251] Fix missing AssaultPropertiesUpdate validation.
 // - https://github.com/codecentric/chaos-monkey-spring-boot/pull/xxx[#xxx] Added example entry. Please don't remove.

 === Improvements
 // - https://github.com/codecentric/chaos-monkey-spring-boot/pull/xxx[#xxx] Added example entry. Please don't remove.

 === New Features
+ - https://github.com/codecentric/chaos-monkey-spring-boot/pull/254[#254] Added timestamps to chaos monkey enable/disable endpoint.


I think it would suit better in the Improvements section of the changelog.

Also please add yourself to the contributors list a few lines below :)

I was debating between the two! The reason I went with new feature since I was adding a new element to Chaos Monkey. Will make the changes!

WtfJoke · 2021-07-30T08:51:38Z

.../src/main/java/de/codecentric/spring/boot/chaos/monkey/endpoints/ChaosMonkeyJmxEndpoint.java

@@ -78,13 +79,15 @@ public String isChaosMonkeyActive() {
  @WriteOperation
  public String enableChaosMonkey() {
    this.chaosMonkeySettings.getChaosMonkeyProperties().setEnabled(true);
-    return "Chaos Monkey is enabled";
+    return "Chaos Monkey is enabled\nActivatedAt:"
+        + Clock.systemDefaultZone().instant().atZone(Clock.systemDefaultZone().getZone());


You can simplify this :)

Suggested change

+ Clock.systemDefaultZone().instant().atZone(Clock.systemDefaultZone().getZone());

+ ZonedDateTime.now();

Now that I see it, I would preferr the term "enabledAt" over "activatedAt" as we already use the verb enabled in "Chaos Monkey is enabled".

I think it would make sense to use StringJoiner, it would make the code more readable:

new StringJoiner("\n") .add("Chaos Monkey is enabled") .add("enabledAt: " + ZonedDateTime.now()) .toString();

However I think even better would be to introduce a new class (POJO, eg. ChaosMonkeyEnabledDto and ChaosMonkeyDisabledDto) and return that instead. Then we dont need to deal with "\n" and formats and spring/jackson can take care of the conversion of that object to a string :)

Then it could look like this:

@WriteOperation public ChaosMonkeyEnabledDto enableChaosMonkey() { this.chaosMonkeySettings.getChaosMonkeyProperties().setEnabled(true); return new ChaosMonkeyEnabledDto(); }

@Data public class ChaosMonkeyEnabledDto { private final String status = "Chaos Monkey is enabled"; private ZonedDateTime enabledAt = ZonedDateTime.now(); }

The class's is a good idea.

That was a fumble on my part. Makes more sense to use that since my complex code just basically invoked ZonedDateTime.now()!

WtfJoke · 2021-07-30T09:30:26Z

...ntric/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRequestScopeRestEndpointIntegration.java

@@ -273,7 +275,7 @@ void postToEnableChaosMonkey() {
        testRestTemplate.postForEntity(baseUrl + "/enable", null, String.class);

    assertEquals(HttpStatus.OK, result.getStatusCode());
-    assertEquals("Chaos Monkey is enabled", result.getBody());
+    assertTrue(Objects.requireNonNull(result.getBody()).startsWith("Chaos Monkey is enabled"));


As soon as we introduced an object (see comment above). You could make assertions again.
it would be great if we make a basic assertion that the correct time would be returned.

We could for example remember the time before we send the request and make an assertion that the time is returned should be later than the remembered time.

Yeah it could be two asserts. One which asserts the text/status "Chaos Monkey is enabled" and one the time yes

MatthewBerk · 2021-07-31T22:05:33Z

...e/codecentric/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRequestScopeJmxEndpointTest.java

-    assertThat(chaosMonkeyJmxEndpoint.disableChaosMonkey(), startsWith("Chaos Monkey is disabled"));
+    ZonedDateTime disabledAt = ZonedDateTime.now();
+    ZonedDateTime timeReturned = chaosMonkeyJmxEndpoint.disableChaosMonkey().getDisabledAt();
+    assertTrue(timeReturned.isAfter(disabledAt) || timeReturned.isEqual(disabledAt));


The reason I used isAfter and isEqual is because every time I ran the test, except one time, it had isAfter false and isEqual true! So just in case others encounter that odd event, I didn't want them to get a failed test!

You could use assertIsAfterOrEqualsTo(disabledAt) its a bit more fluent to read

Suggested change

assertTrue(timeReturned.isAfter(disabledAt) || timeReturned.isEqual(disabledAt));

assertThat(timeReturned).isAfterOrEqualTo(disabledAt);

WtfJoke

Almost there! 👯
Looks pretty good so far. 👍 Have only some smaller remarks on the Tests. After that this can get merged :)

WtfJoke · 2021-07-31T22:45:30Z

...e/codecentric/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRequestScopeJmxEndpointTest.java

-    assertThat(chaosMonkeyJmxEndpoint.disableChaosMonkey(), startsWith("Chaos Monkey is disabled"));
+    ZonedDateTime disabledAt = ZonedDateTime.now();
+    ZonedDateTime timeReturned = chaosMonkeyJmxEndpoint.disableChaosMonkey().getDisabledAt();
+    assertTrue(timeReturned.isAfter(disabledAt) || timeReturned.isEqual(disabledAt));


You could use assertIsAfterOrEqualsTo(disabledAt) its a bit more fluent to read

Suggested change

assertTrue(timeReturned.isAfter(disabledAt) || timeReturned.isEqual(disabledAt));

assertThat(timeReturned).isAfterOrEqualTo(disabledAt);

WtfJoke · 2021-07-31T22:59:05Z

...ntric/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRequestScopeRestEndpointIntegration.java


    assertEquals(HttpStatus.OK, result.getStatusCode());
-    assertEquals("Chaos Monkey is enabled", result.getBody());
+    assertTrue(Objects.requireNonNull(result.getBody()).getEnabledAt().isAfter(enabledAt));


It would make sense to assert the status and the enabledAt.
And enabledAt I would do the same like suggested above with assertIsAfterOrEqualsTo

WtfJoke · 2021-07-31T22:59:14Z

...ntric/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRequestScopeRestEndpointIntegration.java


    assertEquals(HttpStatus.OK, result.getStatusCode());
-    assertEquals("Chaos Monkey is disabled", result.getBody());
+    assertTrue(Objects.requireNonNull(result.getBody()).getDisabledAt().isAfter(disabledAt));


The same as above :)

WtfJoke · 2021-07-31T22:59:48Z

...e/codecentric/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRequestScopeJmxEndpointTest.java

-    assertThat(chaosMonkeyJmxEndpoint.enableChaosMonkey(), is("Chaos Monkey is enabled"));
+    ZonedDateTime enabledAt = ZonedDateTime.now();
+    ZonedDateTime timeReturned = chaosMonkeyJmxEndpoint.enableChaosMonkey().getEnabledAt();
+    assertTrue(timeReturned.isAfter(enabledAt) || timeReturned.isEqual(enabledAt));


Suggested change

assertTrue(timeReturned.isAfter(enabledAt) || timeReturned.isEqual(enabledAt));

assertThat(timeReturned).isAfterOrEqualTo(enabledAt);

It says method doesn't exist when did a quick check!

oh my bad. The method name is a bit different. Its isAfterOrEqualTo without the assert in its name

Cannot resolve method 'assertThat(java.time.ZonedDateTime)'

I see why, we usually use assertj, but in this test hamcrest is still used (a similar method there is named greaterThanOrEqualTo).
Assertj has this method, hamcrest not.

I thought we got rid of hamcrest completly, but it seems a few tests still uses it.

You can keep it that way it is or change the whole test to assertj, I'll leave it up to you.

I most likely would do a PR afterwards which removes hamcrest completly (as it's causes such confusions like here :D)

If you keep it that way, just resolve all the comments related to isAfterOrEqualTo.
I think the only open comments would be the one in ChaosMonkeyRequestScopeRestEndpointIntegration.java

I updated it! Though I left import static org.hamcrest.MatcherAssert.assertThat; since the other tests needed it!

WtfJoke

LGTM 🕐

Thank you @MatthewBerk

WtfJoke reviewed Jul 30, 2021

View reviewed changes

Added enable/disable endpoint timestamps codecentric#246

4193dc6

MatthewBerk force-pushed the endpoint-timestamps branch from 0505ea1 to 4193dc6 Compare July 31, 2021 19:59

Improved enable/disable endpoint timestamps

f5b7e06

MatthewBerk commented Jul 31, 2021

View reviewed changes

WtfJoke reviewed Jul 31, 2021

View reviewed changes

Edited enable/disable endpoint asserts

1245160

WtfJoke approved these changes Aug 3, 2021

View reviewed changes

WtfJoke merged commit b55684b into codecentric:main Aug 3, 2021

WtfJoke mentioned this pull request Aug 6, 2021

Enable/Disable Endpoint should show timestamp #246

Closed

MatthewBerk mentioned this pull request Oct 27, 2021

Enable/Disable Endpoint should show duration #258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable/Disable Endpoint timestamps #254

Enable/Disable Endpoint timestamps #254

MatthewBerk commented Jul 29, 2021

codecov-commenter commented Jul 29, 2021 •

edited

WtfJoke left a comment

WtfJoke Jul 30, 2021

MatthewBerk Jul 31, 2021

WtfJoke Jul 30, 2021 •

edited

MatthewBerk Jul 31, 2021

WtfJoke Jul 30, 2021

WtfJoke Jul 30, 2021

MatthewBerk Jul 31, 2021 •

edited

WtfJoke Jul 30, 2021

WtfJoke Jul 31, 2021

MatthewBerk Jul 31, 2021

WtfJoke Jul 31, 2021 •

edited

WtfJoke left a comment •

edited

WtfJoke Jul 31, 2021 •

edited

WtfJoke Jul 31, 2021 •

edited

WtfJoke Jul 31, 2021

WtfJoke Jul 31, 2021 •

edited

MatthewBerk Jul 31, 2021

WtfJoke Aug 1, 2021 •

edited

MatthewBerk Aug 1, 2021

WtfJoke Aug 1, 2021 •

edited

WtfJoke Aug 1, 2021 •

edited

MatthewBerk Aug 1, 2021

WtfJoke left a comment

	+ Clock.systemDefaultZone().instant().atZone(Clock.systemDefaultZone().getZone());
	+ ZonedDateTime.now();

	assertTrue(timeReturned.isAfter(disabledAt) \|\| timeReturned.isEqual(disabledAt));
	assertThat(timeReturned).isAfterOrEqualTo(disabledAt);

	assertTrue(timeReturned.isAfter(enabledAt) \|\| timeReturned.isEqual(enabledAt));
	assertThat(timeReturned).isAfterOrEqualTo(enabledAt);

Enable/Disable Endpoint timestamps #254

Enable/Disable Endpoint timestamps #254

Conversation

MatthewBerk commented Jul 29, 2021

codecov-commenter commented Jul 29, 2021 • edited

Codecov Report

WtfJoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke Jul 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewBerk Jul 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke Jul 31, 2021 • edited

Choose a reason for hiding this comment

WtfJoke left a comment • edited

Choose a reason for hiding this comment

WtfJoke Jul 31, 2021 • edited

Choose a reason for hiding this comment

WtfJoke Jul 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke Jul 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke Aug 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke Aug 1, 2021 • edited

Choose a reason for hiding this comment

WtfJoke Aug 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 29, 2021 •

edited

WtfJoke Jul 30, 2021 •

edited

MatthewBerk Jul 31, 2021 •

edited

WtfJoke Jul 31, 2021 •

edited

WtfJoke left a comment •

edited

WtfJoke Jul 31, 2021 •

edited

WtfJoke Jul 31, 2021 •

edited

WtfJoke Jul 31, 2021 •

edited

WtfJoke Aug 1, 2021 •

edited

WtfJoke Aug 1, 2021 •

edited

WtfJoke Aug 1, 2021 •

edited