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

Telemetry health checks #135

Closed
adamlc opened this issue Apr 15, 2016 · 9 comments
Closed

Telemetry health checks #135

adamlc opened this issue Apr 15, 2016 · 9 comments
Labels

Comments

@adamlc
Copy link

adamlc commented Apr 15, 2016

Its great to see theres now telemetry. I think a couple of small updates need to be made. I see that the telemetry service registers a health check in consul, which is fine but it doesn't seem like the name is configurable and it seems the healthcheck doesn't get deleted when the container stops.

Should hopefully be changed easily enough :)

@tgross
Copy link
Contributor

tgross commented Apr 18, 2016

it seems the healthcheck doesn't get deleted when the container stops

That's definitely a bug... I'll see what I can do about tracking it down.

@tgross tgross added the bug label Apr 18, 2016
@misterbisson
Copy link
Contributor

it doesn't seem like the [telemetry service] name is configurable

True. The telemetry service name is not configurable, as it represents ContainerPilot itself. The roadmap for the telemetry service includes presenting additional metadata about the container. Predictability in the service name is intended as a way to simplify the development of consumers and new thinking about how this data can be used.

The service ​_does_​ support filtering using tags. You can add tags to the ContainerPilot config and look for them using the consul_sd_config options, even though that's not demonstrated in https://github.com/autopilotpattern/prometheus.

the healthcheck doesn't get deleted when the container stops

We agree, that's a bug we need to fix.

@adamlc
Copy link
Author

adamlc commented Apr 19, 2016

The telemetry service name is not configurable, as it represents ContainerPilot itself. The roadmap for the telemetry service includes presenting additional metadata about the container.

That makes sense, I guess a consumer would just query all of the ContainerPilot services 😸

@tgross
Copy link
Contributor

tgross commented May 12, 2016

I need to get back to this one so I'm going to take a look at it this morning. We did a big refactoring of the config parsing since this issue was opened so the problem may have changed/fixed because of the way we're creating a service for the telemetry endpoint.

@tgross
Copy link
Contributor

tgross commented May 12, 2016

I've opened #155 with an integration test exercising the registration and deregistration of the telemetry endpoint in Consul. I can't reproduce this bug with this test or with "by hand" tests, at least with the latest version of ContainerPilot.

Can you confirm this is still a problem @adamlc and maybe provide a Compose file and ContainerPilot config that reproduces the issue if so?

@adamlc
Copy link
Author

adamlc commented May 12, 2016

I'll take another look in the next few days @tgross 👍

@tgross tgross removed the open PR label May 12, 2016
@justenwalker
Copy link
Contributor

justenwalker commented May 13, 2016

@adamlc One thing I've seen that makes de-registration break is if you are using a consul cluster and registering/deregistering by DNS name that contains more than one entry (ie, one for each server in your cluster: consul.company.com:8500 ). It is best if you register your service against a local agent that is joined to the consul cluster as a client, rather than the consul servers directly. This is a problem regardless of being a telemetry service, or a container service.

@adamlc
Copy link
Author

adamlc commented May 18, 2016

Sorry about the late reply guys, I've not been able to look at this yet! To be honest I was messing about locally so it could well be what @justenwalker said :)

@misterbisson
Copy link
Contributor

@adamlc I'm going to close this one, but please re-open if you see the problem again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants