Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor/monitor #1699

Merged
merged 12 commits into from Jun 22, 2018
Merged

Refactor/monitor #1699

merged 12 commits into from Jun 22, 2018

Conversation

mercul3s
Copy link
Contributor

What is this change?

Adds an etcd backed monitor implementation.

Why is this change necessary?

Closes #1674

Does your change need a Changelog entry?

Maybe not yet? This code is not yet integrated and doesn't change any system behavior. Happy to add one if necessary, or can update the changelog when integrated.

Do you need clarification on anything?

Nope!

Were there any complications while making this change?

This refactor exists alongside the current implementation, and shares some naming. Due to that, there are a few long names for functions or variables that exist in both files that should be changed when we factor out the in-memory monitor.

Have you reviewed and updated the documentation for this change? Is new documentation required?

Nope!

Copy link
Contributor

@echlebek echlebek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward to seeing this replacing this our existing monitor. 💪

// Service is the monitors interface.
type Service interface {
// GetMonitor starts a new monitor.
GetMonitor(ctx context.Context, name string, entity *types.Entity, event *types.Event, ttl int64) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could select a better name than GetMonitor. The method returns an error, so it doesn't actually get a monitor.

What about RefreshMonitor? It seems like the purpose of this method is actually to reset the TTL of a monitor, whether or not it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable to me.

monitorKeyBuilder = store.NewKeyBuilder(monitorPathPrefix)
)

// EtcdGetter provides access to the etcd client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be unused

e(err)
}

// EtcdService is an etcd backend monitor service based on leased keys. Each key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this sentence would be more clear if it were just:

EtcdService is an etcd backend monitor service based on leased keys. Each key
has a watcher that waits for a DELETE or PUT event and calls a handler.

require.NoError(t, err)
response, err := client.Get(context.Background(), monitorPath)
require.NoError(t, err)
fmt.Println("put response in test:", response)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray Println

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♀️

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it happens 🤷‍♀️

assert.EqualValues(t, testMon.key, mon.key)
}

func TestWatchMonDelete(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that these tests don't rely on assertions, they probably would benefit from some comments explaining them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew I was forgetting something.

Copy link

@nikkictl nikkictl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent test cases.. great work 💯Just those 2 remaining comments and you're good to go 😎

require.NoError(t, err)
response, err := client.Get(context.Background(), monitorPath)
require.NoError(t, err)
fmt.Println("put response in test:", response)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it happens 🤷‍♀️

Copy link
Contributor

@echlebek echlebek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few more doc changes I'd like to see, then SHIP IT 🚢 :shipit: 👍


// Service is the monitors interface.
type Service interface {
// RefreshMonitor starts a new monitor.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// RefreshMonitor starts a new monitor or resets an existing monitor

Would that be more accurate?

RefreshMonitor(ctx context.Context, name string, entity *types.Entity, event *types.Event, ttl int64) error
}

// Factory takes an entity and returns a Monitor interface so the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs should be a little more truthy here :)

HandleFailure(entity *types.Entity, event *types.Event) error
}

type ErrorHandler interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs docs

HandleError(error)
}

// ErrorHandler provides a handler for errors from WatchMon.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrorHandlerFunc implements ErrorHandler

@nikkictl
Copy link

How did "Don't break the build" actually break the build 😆

go : # github.com/sensu/sensu-go/backend/monitor
166At C:\gopath\src\github.com\sensu\sensu-go\build.ps1:215 char:5
167+     go test -timeout=200s -tags=integration $(go list ./... | Select- ...
168+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
169    + CategoryInfo          : NotSpecified: (# github.com/se...backend/monitor:String) [], RemoteException
170    + FullyQualifiedErrorId : NativeCommandError
171 
172backend\monitor\monitors_test.go:98:9: no new variables on left side of :=

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Implementation is based on a leased key and watcher per monitor.
GetMonitor either creates a key or extends a ttl, and starts a watch
operation that is set to trigger a handler when a DELETE operation is
called in etcd.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Monitors are based on a leased key in etcd. A watcher waits for a DELETE
event and fires a failure handler when that event is seen. If a key has
a PUT event, its watcher is terminated to avoid multiple watchers on the
same key.

Co-authored by: Mercedes Coyle <mercedes@sensu.io>
Co-authored by: Eric Chlebek eric@sensu.io

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Co-authored by: Mercedes Coyle <mercedes@sensu.io>
Co-authored by: Eric Chlebek eric@sensu.io

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Tested getMonitor and GetMonitor. Removed TestMonitorsHandleUpdate as it
seemed ineffective.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Added tests to verify GetMonitor creates new monitors and extends TTLs.
Removed transaction for put since we are occasionally modifying the
keys.

Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
Signed-off-by: Mercedes Coyle <mercedes@sensu.io>
@mercul3s mercul3s merged commit 20d5a39 into master Jun 22, 2018
@mercul3s mercul3s deleted the refactor/monitor branch June 22, 2018 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants