Skip to content

Node and Pod Metrics#192

Merged
r-dilip merged 7 commits intofeature/customMetricsfrom
dilipr/nodePodMetrics2
Feb 14, 2019
Merged

Node and Pod Metrics#192
r-dilip merged 7 commits intofeature/customMetricsfrom
dilipr/nodePodMetrics2

Conversation

@r-dilip
Copy link
Contributor

@r-dilip r-dilip commented Feb 13, 2019

No description provided.

@r-dilip r-dilip requested a review from a team February 13, 2019 21:43

<match oms.api.ContainerNodeInventory**>
type out_oms_api
<match oms.containerinsights.ContainerNodeInventory**>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to merge ci_feature into feature/customMetrics. That is why this change is seen. I merged from ci_feature into my topic branch

@log.debug "After check_custom_metrics_availability process_incoming_stream #{@process_incoming_stream}"
end

def check_custom_metrics_availability
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is in multiple places now. Good reason to put it as a library & ref it..

"data": {
"baseData": {
"metric": "%{metricName}",
"namespace": "insights.container/nodes",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this go to 'pods' namespace instead of 'nodes' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that will create too many namespaces. I think it makes sense to have one single namespace for container insights. This will also cause discoverability issues. We can't expect customers to go check every namespace to see what metrics each one has. Just my thoughts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1 ) There are not much restrictions on the num namespaces
#2) So you expect customers to look for podcount under /nodes (and will make it easier to discover than putting them under 'pods' ? In the futuire when we add pod cpu, we will add it under nodes? then how about node cpu ? If i am looking for charting anything podlevel its easy to understand to go to pods namespace.
3) either flatten everything and put under default namespace or catagorize it properly.

containerNodeInventoryRecord['DockerVersion'] = dockerVersion
# ContainerNodeInventory data for docker version and operating system.
containerNodeInventoryEventStream.add(emitTime, containerNodeInventoryRecord) if containerNodeInventoryRecord
containerNodeInventoryWrapper = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this change. Since the PR was created before the target branch was updated, this shows up.

@r-dilip r-dilip merged commit 337d35d into feature/customMetrics Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants