Remove node_name from k8s_pod and k8s_container monitored resources. #50
Conversation
There was a problem hiding this comment.
A few comments on kubernetes.cc. These are on unedited lines, so commenting at the top level:
L56: kK8sNodeResourcePrefix is no longer used.
L44: kKubernetesApiVersion -> why is this fixed to 1.6? Is there a way to know the current kubernetes version?
|
Responding to @supriyagarg:
Removed.
Not really. That said, we talked about tagging this with the actual API version (e.g., "v1", "v1beta1", etc). Out of scope for this PR, though. |
| {"topLevelControllerType", json::string(top_level_kind)}, | ||
| {"topLevelControllerName", json::string(top_level_name)}, | ||
| })}, | ||
| {"nodeName", json::string(node_name)}, |
There was a problem hiding this comment.
Is it a common convention to use camel casing for metadata labels vs underscores for resource labels? Is there any value in trying to push for consistency?
There was a problem hiding this comment.
The monitored resource labels use snake_case. Since the associations are part of the metadata blobs, we've tried to be consistent with the source of the metadata -- in this case, the Kubernetes API. Kubernetes uses camelCase, so we stuck with that.
| : "Pod"; | ||
|
|
||
| const json::Object* spec = pod->Get<json::Object>("spec"); | ||
| const std::string node_name = spec->Get<json::String>("nodeName"); |
There was a problem hiding this comment.
What happens when node name is not set? Is the value null or empty?
There was a problem hiding this comment.
If node name is not set, this will throw a json::Exception. However, given that our query filters the pods that run on this node, those pods will definitely have a node name.
That said, we should figure out what to do about node-less (unscheduled) pods. I've added a comment.
| : "Pod"; | ||
|
|
||
| const json::Object* spec = pod->Get<json::Object>("spec"); | ||
| const std::string node_name = spec->Get<json::String>("nodeName"); |
There was a problem hiding this comment.
If node name is not set, this will throw a json::Exception. However, given that our query filters the pods that run on this node, those pods will definitely have a node name.
That said, we should figure out what to do about node-less (unscheduled) pods. I've added a comment.
| {"topLevelControllerType", json::string(top_level_kind)}, | ||
| {"topLevelControllerName", json::string(top_level_name)}, | ||
| })}, | ||
| {"nodeName", json::string(node_name)}, |
There was a problem hiding this comment.
The monitored resource labels use snake_case. Since the associations are part of the metadata blobs, we've tried to be consistent with the source of the metadata -- in this case, the Kubernetes API. Kubernetes uses camelCase, so we stuck with that.
3 participants
No description provided.