-
Notifications
You must be signed in to change notification settings - Fork 381
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
Doc: Tetragon metrics #1495
Doc: Tetragon metrics #1495
Conversation
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thank you @prateek041 for this PR. Some high-level comments I have:
|
Thank you @lambdanis for the review, making changes now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @prateek041 as long as it works let's please merge it
|
||
Tetragon, when installed via release packages as mentioned in [Package Deployment](../getting-started/deployment/package.md). It can be interacted with `tetragon` command. By default, metrics are disabled, which can be enabled using `--metrics-server` flag, by specifying the address. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @prateek041 please add that you can set the metrics address server by creating a file under /etc/tetragon/tetragon.conf.d/metrics-server
with the address server as content, example localhost:2112
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also please reference https://tetragon.cilium.io/docs/reference/tetragon-configuration/#configuration-examples for further examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is done right? Beside container stuff that should be contributed in another PR
This is the part that is still missing right? can you do it in separate PR, please so we can merge this and have the metrics explained maybe in the reference? @jrfastab @michi-covalent wdyt reference for metrics is better? |
You have one CI link failure I guess at the end you do package.md should be just package/ without |
yeah let's do a separate PR |
Let me make the suggested changes and push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you @prateek041 for the updates.
- Explain Tetragon metrics
This is the part that is still missing right? can you do it in separate PR, please so we can merge this and have the metrics explained maybe in the reference?
Tetragon will have an autogenerated metrics reference soon™, so there is no need to explain metrics in this PR. Unless you have a specific idea what needs an extra explanation in addition to the reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to do this. Sorry if it looks overwhelming, I have many comments this time. Please tell me if you don't understand any and need clarification. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please can you wrap the lines at ~80 chars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sorry for that, I had soft-wrap on, so it appeared Ok on my IDE. fixed that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries
### Fetch the Metrics | ||
|
||
Then the exposed metrics can be fetched using `curl` command at `localhost:2112/metrics` | ||
|
||
```shell | ||
➜ ~ curl localhost:2112/metrics | ||
|
||
# HELP promhttp_metric_handler_errors_total Total number of internal errors encountered by the promhttp metric handler. | ||
# TYPE promhttp_metric_handler_errors_total counter | ||
promhttp_metric_handler_errors_total{cause="encoding"} 0 | ||
promhttp_metric_handler_errors_total{cause="gathering"} 0 | ||
# HELP tetragon_errors_total The total number of Tetragon errors. For internal use only. | ||
# TYPE tetragon_errors_total counter | ||
... | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of repeating this, we can factorize the way to reach the metrics endpoint, maybe with tabs or simply as you did with section headers and then have a single "fetch the metrics" instead of identical ones like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, added a "Fetch the metrics" section at the end, please take a look, and tell if it needs to be changed.
by the way, we all reviewed while this was still a draft, please put it in ready for review when you think it's good. |
Oh that's great @lambdanis, please ask me to review the PR when you do this as we need to exclude some generated files from the ownership code review thingie Lines 16 to 18 in 22a9fe8
And also I'm curious about scripts, we have two at the moment for this purpose: |
caf7d89
to
1ee9234
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So thanks for all these changes, still 2 more things I noted them, please do fix then from my part acke'ed thanks ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two suggestions to make things clearer, otherwise looks good.
Added how to read tetragon metrics with Kubernetes and Package installation setup. Signed-off-by: Prateek Singh <prateeksingh9741@gmail.com>
1ee9234
to
ee14431
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. In the future I think a tutorial running Prometheus and scraping metrics might be helpful, but this is good as the first iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with the actual state, we can merge if there are no more remarks!
Closes #928
This PR: