-
Notifications
You must be signed in to change notification settings - Fork 136
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
Breakout packages #123
Breakout packages #123
Conversation
Making PollAction a method of Pollable will allow us to avoid a circular import when we split out packages from the main containerbuddy package.
Packages that include Pollables are going to want to validation their own configuration, so by moving the parsing and IPs functions into a utils package we can do so without any circular references.
Making PollAction a method of Pollable will allow us to avoid a circular import when we split out packages from the main containerbuddy package.
Packages that include Pollables are going to want to validation their own configuration, so by moving the parsing and IPs functions into a utils package we can do so without any circular references.
Created a metrics package that leans on the official Prometheus client to serve metrics. Adds a new Pollable for Sensors that record metrics received from user- defined functions.
MustRegister panics rather than returning an error (thanks!) if we register an invalid collector, even just an invalid name. This would give end-users a big ugly golang stack trace that they have to dig the error message out of. So we'll catch the panic so we can return an error.
Also moved the similar reset we do for Service.CheckHealth and Backend.OnChange into defers so that we can ensure they happen even if the function errors-out. Includes tests and new testdata script for metrics package.
Unit tests of recording metrics for each of the four collector types and fetching them via the web API. Cleaned up some of the existing tests so that metrics names are more directly tied-back to test cases. Fixed a bug in `Sensor.record()` where the type switch doesn't work because the prometheus collector structs are actually interfaces. So we end up having to do some ugly string checking.
I'm wondering whether maybe we change |
Huzzah, all tests pass! |
Added documentation for metrics feature. The README is getting quite long, so while anticipating our package breakout I split the metrics README into the metrics directory and linked to it from the front page. I expect we'll do something along these lines for other sections in the future.
I can see how this would be painful. I still think we are complicating the idea of external configuration (ie from a file) with our application state (internal configuration) - I believe the fact that the structures are very similar is incidental. Perhaps this isn't the time to undertake such a refactor, but does the idea of separating these concepts from each other seem like a good goal? Maybe we can create another issue for that. Here are some of my thoughts: instead of // Config is the top-level Containerbuddy Configuration
type Config struct {
Consul string `json:"consul,omitempty"`
Etcd json.RawMessage `json:"etcd,omitempty"`
LogConfig *LogConfig `json:"logging,omitempty"`
OnStart json.RawMessage `json:"onStart,omitempty"`
PreStart json.RawMessage `json:"preStart,omitempty"`
PreStop json.RawMessage `json:"preStop,omitempty"`
PostStop json.RawMessage `json:"postStop,omitempty"`
StopTimeout int `json:"stopTimeout"`
Services []*services.ServiceConfig `json:"services"`
Backends []*backends.BackendConfig `json:"backends"`
PreStartCmd *exec.Cmd
PreStopCmd *exec.Cmd
PostStopCmd *exec.Cmd
Command *exec.Cmd
QuitChannels []chan bool
ConfigFlag string
} something like type ApplicationState struct {
PreStartCmd *exec.Cmd
PreStopCmd *exec.Cmd
PostStopCmd *exec.Cmd
Command *exec.Cmd
QuitChannels []chan bool
Backends []backends.Backend
Service []services.Service
ConfigFlag string
} Inside For instance, after loading up the More concrete steps:
In retrospect I realize I oversimplified, this does not account for the fact that we don't re-run the shimmed application and preStart commands but it is part of the ApplicationState. |
I think you're absolutely on the right track here, and I think this sounds like a good approach. It's not that different than what I've got here already but is semantically a lot cleaner. I'm open to the idea of trying to get that idea in, but I'm also trying to ship metrics sooner rather than later. So I'll take a little bit of time today to see how much of a stretch it would be to roll that into this PR. If it's not too much of a hassle I think we can get it in, otherwise would you be open to keeping #83 open for continuing the refactor in stages? |
@justenwalker I've had another chance to look at this and it's probably another day's work. I'm a bit pressed for time because I've got some otherwise unrelated work pushing to try to get the metrics stuff in. So here's my proposal:
|
Sensors were polling but the telemetry server was blocking its thread because we didn't put it into its own goroutine. Also needed to take a pointer for PollAction on Sensor so that we can update the exec/Cmd struct after execution. Added an integration test exercising these.
@justenwalker @misterbisson I've updated this PR with the following:
What I'd like to do is cut this branch as 1.4.0-rc3 so that we can incorporate it into autopilotpattern/touchbase (ref autopilotpattern/touchbase#18). If we're happy with that, then I'd like to merge this more or less as-is (modulo any review comments). This exercise got a little bit away from us but I think it's better that we have it done now so that future projects aren't harder to implement. The only thing that isn't yet done here from the above discussion is completely separating out the |
@@ -58,7 +58,7 @@ docker: build/containerbuddy_build consul etcd | |||
|
|||
# top-level target for vendoring our packages: godep restore requires | |||
# being in the package directory so we have to run this for each package | |||
vendor: containerbuddy/vendor | |||
vendor: backends/vendor config/vendor core/vendor discovery/vendor services/vendor utils/vendor telemetry/vendor |
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.
I'm pretty sure we're abusing godep here - I have never seen any project contain several godep manifests and vendor folders in sub modules - I don't this will scale well.
Maybe we can just play with the mount locations and move vendor/godeps back to the top-level?
When doing a SIGHUP to reload the config - I get this error:
|
I tried fixing up the makefile to remove the redundant godeps check it out here: https://github.com/justenwalker/containerbuddy/tree/fix-makefile Update: that branch is failing tests because I made lint warnings into errors. If we don't want that to break tests, it would be an easy fix to remove that feature" One thing I discovered while doing this: |
The problem I was trying to solve there is that this breaks using the subpackages independently. If you tried to Honestly, I'm fine with that and feel like the packages should just be an organizational thing rather than trying to optimize for a corner case. Your approach looks good to me.
Edit: I see what you're doing -- you're mounting the make file inside the container. I think we can just call bash directly without calling |
Good catch. Will try to fix. |
@justenwalker I've merged your changes and tweaked so as to eliminate the extra make indirection. Going to try to fix the SIGHUP problem next. |
Yeah - sorry I was being "clever". If we want to avoid using make at all inside the container that's fine - the main benefit of make is it's ability to use file modification dates to avoid doing work - since the containers are destroyed after execution, it doesn't really do anything useful. |
I never thought of that. I did run unto a potential problem using that method though: When you make a method in |
I passed I think I've got the SIGHUP bug licked, but just trying to make sure our tests exercise it correctly. |
I thought wrong... the problem I'm running into is that our HTTP server doesn't have any way to gracefully reload. There are two libraries I'm examining now to do this. |
Ok @justenwalker I think I've got that figured out. There's a minor caveat to that which is that we can't change the telemetry port. But I'd like to revisit that under a separate body of work when we expand the server with things like #27 (comment) |
🏡 🚶 |
I've got this tested-out with https://github.com/autopilotpattern/touchbase and we're feeling good about merging this in finally. |
@justenwalker per our discussion in #83 this expands on and includes #118.
Unit tests pass and I'm working on the build integration tests but I'd like to see what you think of this approach. Note that "NewService" ends up being a no-so-optimal approach just because we don't want to change the JSON syntax and injecting the discovery service became super painful. I'd certainly be open to suggestions for improvement here though.
cc @misterbisson just as an FYI