Add instance association to Docker containers. #59
Conversation
Refactor the Docker code to look more uniform with Kubernetes.
| #endif | ||
| constexpr const char docker_endpoint_path[] = "/containers"; | ||
| constexpr const char kDockerEndpointPath[] = "/containers"; | ||
| constexpr const char kDockerContainerResourcePrefix[] = "container"; |
There was a problem hiding this comment.
Perhaps use a more descriptive prefix like "docker_container" to differentiate from "k8s_container" and "gke_container"
There was a problem hiding this comment.
Unfortunately, this string is user-visible, and we're already stuck with it. I'm just moving it from a hard-coded string into a constant.
igorpeshansky
left a comment
igorpeshansky
left a comment
There was a problem hiding this comment.
Thanks, PTAL.
| #endif | ||
| constexpr const char docker_endpoint_path[] = "/containers"; | ||
| constexpr const char kDockerEndpointPath[] = "/containers"; | ||
| constexpr const char kDockerContainerResourcePrefix[] = "container"; |
There was a problem hiding this comment.
Unfortunately, this string is user-visible, and we're already stuck with it. I'm just moving it from a hard-coded string into a constant.
src/docker.cc
Outdated
| const std::string id = container->Get<json::String>("Id"); | ||
| // Inspect the container. | ||
| try { | ||
| json::value raw_docker = QueryDocker( |
There was a problem hiding this comment.
Being that we're querying for metadata, raw_metadata, raw_container, or raw_container_metadata may be clearer.
There was a problem hiding this comment.
Renamed to raw_container (raw_metadata was used for the object sent to the API).
| MetadataAgent::Metadata::IGNORED() | ||
| #endif | ||
| ); | ||
| } catch (const QueryException& e) { |
There was a problem hiding this comment.
Just a thought, now that we know what we know with the cpp-netlib, and that non 200 responses aren't actually throwing exceptions, the reason for this is more likely to be an issue with the docker daemon or the server it's running, not that the container disappears.
There was a problem hiding this comment.
Yes, I plan to fix status code handling in a subsequent PR, but it would be easier with this refactoring submitted.
| json::value parsed_list = QueryDocker( | ||
| std::string(kDockerEndpointPath) + "/json?all=true" + container_filter); | ||
| Timestamp collected_at = std::chrono::system_clock::now(); | ||
| if (config_.VerboseLogging()) { |
There was a problem hiding this comment.
Why did you remove this verbose logging? Is it only helpful during development?
There was a problem hiding this comment.
This verbose logging is now part of QueryDocker.
| #ifdef VERBOSE | ||
| LOG(DEBUG) << "QueryDocker: Response: " << body(response); | ||
| #endif | ||
| return json::Parser::FromString(body(response)); |
There was a problem hiding this comment.
This is the area I'm referring to for non 200 responses. This is likely work for the future, but we should be checking for the response status code, instead of just assuming we're returning the right response based on ANY response at all.
There was a problem hiding this comment.
Yes, I have a PR ready to go with a fix, just waiting to rebase on this refactoring.
igorpeshansky
left a comment
igorpeshansky
left a comment
There was a problem hiding this comment.
Thanks, PTAL.
src/docker.cc
Outdated
| const std::string id = container->Get<json::String>("Id"); | ||
| // Inspect the container. | ||
| try { | ||
| json::value raw_docker = QueryDocker( |
There was a problem hiding this comment.
Renamed to raw_container (raw_metadata was used for the object sent to the API).
| MetadataAgent::Metadata::IGNORED() | ||
| #endif | ||
| ); | ||
| } catch (const QueryException& e) { |
There was a problem hiding this comment.
Yes, I plan to fix status code handling in a subsequent PR, but it would be easier with this refactoring submitted.
| json::value parsed_list = QueryDocker( | ||
| std::string(kDockerEndpointPath) + "/json?all=true" + container_filter); | ||
| Timestamp collected_at = std::chrono::system_clock::now(); | ||
| if (config_.VerboseLogging()) { |
There was a problem hiding this comment.
This verbose logging is now part of QueryDocker.
| #ifdef VERBOSE | ||
| LOG(DEBUG) << "QueryDocker: Response: " << body(response); | ||
| #endif | ||
| return json::Parser::FromString(body(response)); |
There was a problem hiding this comment.
Yes, I have a PR ready to go with a fix, just waiting to rebase on this refactoring.
3 participants
Refactor the Docker code to look more uniform with Kubernetes.