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

Add add Automated Dashboard for Kubernetes Node metrics #64

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

jeffkreeftmeijer
Copy link
Member

AppSignal for Kubernetes extracts node metrics for each node running in a Kubenetes cluster. This automated dashboard displays some of those metrics in an automated dashboard named "Kubernetes Nodes".

AppSignal for Kubernetes extracts node metrics for each node running
in a Kubenetes cluster. This automated dashboard displays some of
those metrics in an automated dashboard named "Kubernetes Nodes".
@jeffkreeftmeijer jeffkreeftmeijer self-assigned this Apr 4, 2024
@jeffkreeftmeijer jeffkreeftmeijer changed the title Add add autmated dashboard for Kubernetes Node metrics Add add Automated Dashboard for Kubernetes Node metrics Apr 4, 2024
Comment on lines +215 to +243
"name": "node_fs_inodes",
"fields": [
{
"field": "GAUGE"
}
],
"tags": [
{
"key": "node",
"value": "*"
}
]
},
{
"name": "node_fs_inodes_free",
"fields": [
{
"field": "GAUGE"
}
],
"tags": [
{
"key": "node",
"value": "*"
}
]
},
{
"name": "node_fs_inodes_used",
Copy link
Member

Choose a reason for hiding this comment

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

Let's use tags for different states like free and used instead of reporting them as different metrics. We do this for other (host) metrics too. It would help that we don't have to show the full metric name then for every line in the graph, freeing up valuable space in the hover box.

For example:

  • Metric name: node_fs_inodes
  • Tags:
    • state
      • Values:
        • free
        • used

I also see this in some other graphs in this dashboard. We should update those as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to keep this dashboard as close to what's reported from Kubernetes as I can. I think this is a good idea for the future, when we know what we'd like to report exactly, but let's get this out of the door and get users to try it first.

{
"title": "Node CPU Usage",
"description": "node_cpu_usage_nano_cores",
"line_label": "%name% %node%",
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't include the metric name unless there are multiple metrics in a graph, and for those graphs we're better off using tags on one metric. See my other comment.

Suggested change
"line_label": "%name% %node%",
"line_label": "%node%",

"visuals": [
{
"title": "Node CPU Usage",
"description": "node_cpu_usage_nano_cores",
Copy link
Member

Choose a reason for hiding this comment

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

These descriptions don't explain anything to me. Let's remove them if it's just for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if we can do human-readable descriptions, let's do those instead!

Copy link
Member Author

Choose a reason for hiding this comment

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

These are the internal metric names, so they'll probably make sense to Kubernetes users.

Copy link
Member

@tombruijn tombruijn Apr 10, 2024

Choose a reason for hiding this comment

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

I can't the names of these metrics anywhere, so I wouldn't be so sure that it's clear. The same way I can't find that you're using the kubernetes metric names, like you say here. In the library I see it being mapped from a JSON struct that doesn't use the same naming: https://github.com/appsignal/appsignal-kubernetes/blob/0b3f39d65ba99622ab3e647e8e4c012ee944baca/src/main.rs#L97-L167

Do you have any links to docs or source code that mentions these metric names?

@roytomeij
Copy link
Member

I'm not sure what I should review :)

@jeffkreeftmeijer jeffkreeftmeijer requested review from thijsc and removed request for roytomeij April 5, 2024 17:48
Copy link
Contributor

@unflxw unflxw left a comment

Choose a reason for hiding this comment

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

If this is just a quick test with Jim and select users, then this is fine.

[But then, maybe we don't need a magic dashboard at all? Maybe we can just give them a JSON dashboard definition that they can manually import into AppSignal?]

Otherwise, what @tombruijn said.

And also maybe we could consider prefixing these metric names with something like k8s_? Otherwise it might be confusing with the Node.js Heap Statistic metrics (there's no overlap, though -- those are nodejs_, not node_)

@jeffkreeftmeijer jeffkreeftmeijer merged commit 2f4dddc into main Apr 8, 2024
1 check passed
@jeffkreeftmeijer jeffkreeftmeijer deleted the kubernetes-nodes branch April 8, 2024 12:38
@jeffkreeftmeijer jeffkreeftmeijer restored the kubernetes-nodes branch April 8, 2024 12:39
@jeffkreeftmeijer
Copy link
Member Author

Accidentally merged, reverted and continuing in #65.

@jeffkreeftmeijer
Copy link
Member Author

If this is just a quick test with Jim and select users, then this is fine.

[But then, maybe we don't need a magic dashboard at all? Maybe we can just give them a JSON dashboard definition that they can manually import into AppSignal?]

We're adding a dashboard to give users a bit of an easier time getting their Kubernetes setup working. This is mostly an example dashboard at this point, but it saves users from having to write their own.

Otherwise, what @tombruijn said.

And also maybe we could consider prefixing these metric names with something like k8s_? Otherwise it might be confusing with the Node.js Heap Statistic metrics (there's no overlap, though -- those are nodejs_, not node_)

You're right, this could be a bit confusing. I'm putting this on the list for later, as we're going to add more metrics that also warrant namespacing (think pod metrics, for example). I'll pick this one up then.

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

Successfully merging this pull request may close these issues.

5 participants