feature/add-a-health-endpoint

[bartekus]

Resolve Add a /health endpoint #5

Signed-off-by: Bartlomiej P Kus [email protected]

[bartekus]

That should do it, sorry for the overhead, but I had to recreate PR @swcurran

[WadeBarnes]

@bartekus, Thanks for the contribution! The only thing that is missing from the issue request is the "meaningful health check on the server's services". As it stands the /health/check endpoint will return success if the web server is running, but that doesn't provide any indication of whether the tail-server is in a working condition.

That said, a health check is better than no health check. I'd be fine to accept and merge, we'd just need to create a new issue to track the addition of the check on the server's services.

Would you like to take a stab at the check, or defer that for another time?

[bartekus]

That's a good idea, let me have a stab at it, no point in merging half-baked implementations!

[bartekus]

I've expanded on the solution to enable extensibility as well as add second endpoint to facilitate setting/stats dump.
I've take the cues from other implementations (nodejs/flask) but python is not my speciality but I did what I could with the help of PhCharm CE.

[WadeBarnes]

Thanks @bartekus. I'll test it out.

[Executioner1939]

Hi @WadeBarnes is there anyway this can be merged? Anything I can do to help? I quite need an HTTP healthcheck in my environment

[WadeBarnes]

I've had a quick look. Overall the updates look fine. Care would need to be taken to ensure the /health/env endpoint was never exposed publicly as it provides too much information about the host system, though I could see it coming in handy for some troubleshooting purposes.

@esune, @loneil, Your thoughts?

[WadeBarnes]

There are some minor conflicts that I can clean up easily to get things up to date with the latest commits.

[esune]

@esune, @loneil, Your thoughts?

For other projects (i.e.: ACA-Py) we protect the sensitive endpoints with the requirement for an api-key or other authentication method. Maybe that's what we should/could do here?

[esune]

@bartekus wondering what the purpose of the env endpoint is, since it discloses information about the host and stack (and to @WadeBarnes point, it might need to be protected if we keep it)

[Executioner1939]

Thanks for looking into this, for now we simply forked the repo and added a basic health check endpoint that just returns a 200 OK as we were in a deadline pinch, however the "fix" is not a proper health / readiness check.

We need this as GKE's Ingress Controller set's up an HTTP(S) Global Loadbalancer which requires that all NEGs pass a GoogleHC before it routes traffic and it can't be disabled.

[bartekus]

You are right, this isn't very good approach, alas I did not revisit this in quite a while, I've rewritten the whole thing in Rust (as learning activity (but never share with anyone)) so perhaps if there's an appetite and further specification re: requirements for the health / readiness check I can implement it and share with the OSS at large

Alternatively I can amend the existing python version, altho its not a language of my passion as rust FYI

[WadeBarnes]

@bartekus, While a rust implementation is intriguing, I think it would be better handled as a completely separate topic. For the scope and context of this PR, removing the EnvDump implementation and associated API would be sufficient to wrap things up.

Let me know if you have the time and desire to handle that, otherwise we can arrange to make the change.