Add LastOk to Event (#1034) #1062

zbintliff · 2018-02-21T22:54:59Z

When Event's status is 0 set LastOk to the timestamp of the event

Signed-off-by: Zach Bintliff zbintliff@gmail.com

What is this change?

Add LastOk to Event type

Why is this change necessary?

Provides feature parity with 1.X sensu.

Closes #1034

Does your change need a Changelog entry?

Yes

Do you need clarification on anything?

First time working with protobuf would appreciate guidance there and if anything looks off
Making sure I merged new and old event correctly so that LastOk lasts properly persists.
The more I look at my testing the more I think it doesn't do anything. So some guidance on how to test would be great.

Were there any complications while making this change?

Following instructions on updating

echlebek

Hi, thanks for the PR! You are the first community member to submit a PR to sensu-go! 🥂 🥇

The PR looks good to me, but please add a gogoproto tag to the proto definition so that the field name would be LastOK. (See below)

echlebek · 2018-02-21T23:03:35Z

types/event.proto

@@ -35,4 +35,7 @@ message Event {

  // Hooks describes the results of multiple hooks; if event is associated to hook execution.
  repeated Hook hooks = 6 [(gogoproto.nullable) = true];
+
+  // LastOk displays last time this check was ok; if event status is 0 this is set to timestamp
+  int64 last_ok = 7;


Let's add [(gogoproto.customname) = "LastOK"] to this field, so that we can get something that fits the Go coding standards. (https://github.com/golang/go/wiki/CodeReviewComments#initialisms)

Yeah I wasn't sure if this follows the ruby name standard last_ok but went with that since the issue specifies. And the Ok vs OK got me too. Thanks

grepory · 2018-02-21T23:15:33Z

The test failure in travis is unrelated to this change, I'm comfortable with merging instead until we fix the tests.

(After addressing PR feedback).

zbintliff · 2018-02-21T23:57:46Z

Updated, not too happy about the test I added as I still don't think it does anything.

grepory

Sorry about the confusion. After looking at this, I realized that LastOK was a little out of place.

grepory · 2018-02-22T00:40:03Z

types/event.proto

@@ -35,4 +35,7 @@ message Event {

  // Hooks describes the results of multiple hooks; if event is associated to hook execution.
  repeated Hook hooks = 6 [(gogoproto.nullable) = true];
+
+  // LastOK displays last time this check was ok; if event status is 0 this is set to timestamp
+  int64 last_ok = 7 [(gogoproto.customname) = "LastOK"];


In this case, we're looking at the Check's status. We'll want to throw LastOK in here.

grepory · 2018-02-22T00:48:55Z

backend/eventd/eventd.go

@@ -158,6 +158,7 @@ func (e *Eventd) handleMessage(msg interface{}) error {
 		}

 		event.Check.MergeWith(prevEvent.Check)
+		event.LastOK = prevEvent.LastOK


Given that we move LastOK from Event to Check, this will have to change. Also, this isn't the only desired effect we want. If the current event's status is OK, we want to use this event's timestamp as LastOK.

This part was just the merge from prevEvent. The current event's LastOK is set in flapping.go here if status is OK.

This will be the same if we move LastOK into the check as well. Except this line will go into the MergeWith()

grepory · 2018-02-22T00:52:54Z

backend/eventd/eventd_test.go

@@ -69,6 +69,7 @@ func TestEventHandling(t *testing.T) {

 	// Make sure the event has been marked with the proper state
 	assert.Equal(t, types.EventPassingState, event.Check.State)
+	assert.Equal(t, badEvent.Timestamp, event.LastOK)


We'll want to assert that event.Check.LastOK is equal to event.Timestamp.

Publishing a bad event seems out of place in this test, but I can look into that another time. Line 63 is where we're publishing our event. We want to make sure that eventd is correctly updating the LastOK field with the current timestamp.

zbintliff · 2018-02-22T02:37:31Z

We can definitely move the LastOK into the Check, but that will be a departure from where it is in the 1.X version and what the #1034 asked for.

I'm also not familiar with the new architecture so not sure if Event in this project is 1:1 with event in the 1.X version. Thanks for any insight.

Edit: After discussion via slack we are moving it into the Check for better representation.

zbintliff · 2018-02-23T00:00:36Z

backend/eventd/eventd_test.go

@@ -69,6 +70,10 @@ func TestEventHandling(t *testing.T) {

 	// Make sure the event has been marked with the proper state
 	assert.Equal(t, types.EventPassingState, event.Check.State)
+	fmt.Printf("Event Timestamp: %d\n", event.Timestamp)
+	fmt.Printf("Check's LastOK: %d\n", event.Check.LastOK)
+	fmt.Printf("badEvent Timestamp: %d\n", badEvent.Timestamp)


@grepory its my understanding that badEvent should have an older timestamp but it doesn't. IF you pull down this branch and run $ go test ./... -run=TestEventHandling -v you'll see they are all the same. Even with modifying FixtureCheck to sleep 5 and print UnixNano(). Am I missing some mutations somewhere?

This was just extra code I was writing to check if my test was valid or not.

Figured it out!

zbintliff · 2018-02-23T00:40:18Z

This should be ready for review again whenever you have time.

portertech · 2018-02-23T00:42:31Z

CHANGELOG.md

@@ -20,6 +20,7 @@ set when importing legacy settings.
 - Configured and enabled etcd autocompaction.
 - Add event metrics type, implementing the Sensu Metrics Format.
 - Added non-functional selections for resolving and silencing to web ui
+- Add LastOk to event type. This will be updated to reflect the last timestamp of a succesful event


"to check type"

grepory · 2018-02-23T00:56:10Z

README.md

@@ -32,7 +32,7 @@ Otherwise, see the **for non-C++ users** [instructions here.](https://github.com

 ### Quick Start

-Once you make a change to any `*.proto` file within the **types** package, you will need regenerate the associated `*.pb.go` file. To do so, simply run the [genproto.sh](https://github.com/sensu/sensu-go/blob/master/scripts/genproto.sh) script, which will install all required dependencies and launch the code generation.
+Once you make a change to any `*.proto` file within the **types** package, you will need regenerate the associated `*.pb.go` file. To do so, simply run the [genproto.sh](https://github.com/sensu/sensu-go/blob/master/scripts/genproto.sh) script, which will install all required dependencies and launch the code generation (be sure to run the below `./build.sh deps` first though).


While you're in there. Can you make it "you will need to regenerate the associated"?

Changes satisfied.

grepory · 2018-02-23T01:12:52Z

Looking into the build failure.

zbintliff · 2018-02-23T01:17:58Z

Great, I'll make that change in the README.md. Do you want me to squash my commits down?

grepory · 2018-02-23T01:27:42Z

@zbintliff Yes, please, if you wouldn't mind. I don't mind doing it for you, but I'd appreciate it. :) We only squash-merge. (I should put that in the contributing doc). If you would be so kind as to reword your initial commit message to be "Add LastOK to Check (#1034)" that would also be preferable.

When a Check's status is 0 set LastOk to the timestamp of the event Signed-off-by: Zach Bintliff <zbintliff@gmail.com>

grepory · 2018-02-23T01:32:53Z

I may have to rebase your branch and re-push, but I'll get this merged for you tonight. I really appreciate your help with this!

grepory requested a review from echlebek February 21, 2018 23:07

echlebek previously requested changes Feb 21, 2018

View reviewed changes

zbintliff force-pushed the feature/lastOk branch 2 times, most recently from b5b6198 to 21fb439 Compare February 21, 2018 23:57

palourde added the ready-for-review label Feb 21, 2018

grepory suggested changes Feb 22, 2018

View reviewed changes

zbintliff commented Feb 23, 2018

View reviewed changes

zbintliff force-pushed the feature/lastOk branch 3 times, most recently from 4bd3afd to d9b1ca5 Compare February 23, 2018 00:37

portertech reviewed Feb 23, 2018

View reviewed changes

grepory approved these changes Feb 23, 2018

View reviewed changes

Add LastOK to Check (sensu#1034)

646af2b

When a Check's status is 0 set LastOk to the timestamp of the event Signed-off-by: Zach Bintliff <zbintliff@gmail.com>

zbintliff force-pushed the feature/lastOk branch from ca41d12 to 646af2b Compare February 23, 2018 01:30

grepory merged commit 23c0728 into sensu:master Feb 23, 2018

grepory removed the ready-for-review label Feb 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LastOk to Event (#1034) #1062

Add LastOk to Event (#1034) #1062

zbintliff commented Feb 21, 2018 •

edited by palourde

echlebek left a comment

echlebek Feb 21, 2018

zbintliff Feb 21, 2018

grepory commented Feb 21, 2018 •

edited

zbintliff commented Feb 21, 2018

grepory left a comment

grepory Feb 22, 2018

grepory Feb 22, 2018

zbintliff Feb 22, 2018

grepory Feb 22, 2018

zbintliff commented Feb 22, 2018 •

edited

zbintliff Feb 23, 2018

zbintliff Feb 23, 2018

zbintliff commented Feb 23, 2018

portertech Feb 23, 2018

grepory Feb 23, 2018

grepory commented Feb 23, 2018

zbintliff commented Feb 23, 2018

grepory commented Feb 23, 2018

grepory commented Feb 23, 2018

Add LastOk to Event (#1034) #1062

Add LastOk to Event (#1034) #1062

Conversation

zbintliff commented Feb 21, 2018 • edited by palourde

What is this change?

Why is this change necessary?

Does your change need a Changelog entry?

Do you need clarification on anything?

Were there any complications while making this change?

echlebek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grepory commented Feb 21, 2018 • edited

zbintliff commented Feb 21, 2018

grepory left a comment

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

Choose a reason for hiding this comment

zbintliff commented Feb 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbintliff commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grepory commented Feb 23, 2018

zbintliff commented Feb 23, 2018

grepory commented Feb 23, 2018

grepory commented Feb 23, 2018

zbintliff commented Feb 21, 2018 •

edited by palourde

grepory commented Feb 21, 2018 •

edited

zbintliff commented Feb 22, 2018 •

edited