-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 label to force containter stats to stream - allowing podman containers to return precpu_stats #37773
base: main
Are you sure you want to change the base?
Conversation
…iners to return precpu_stats
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
❕ Build Aborted
Expand to view the summary
Build stats
Steps errorsExpand to view the steps failures
|
❕ Build Aborted
Expand to view the summary
Build stats
Steps errorsExpand to view the steps failures
|
Sorry, I cannot set them - permission denied :( --> enhancement |
var stream bool | ||
if container.Labels != nil { | ||
if labelValue, ok := container.Labels["co.elastic.metricbeat.docker.forceContainerStatsStream"]; ok { | ||
stream, _ = strconv.ParseBool(labelValue) |
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.
If you set the stream to true , what is the effect on the response time?
Trying it locally the stream=1
parameter in the http client never returns.
curl -s --unix-socket /var/run/docker.sock -H 'Content-Type: application/json' http://localhost/containers/<contId>/stats\?stream\=1
It is like using docker stats
vs docker stats --no-stream
.
When stream=false the docker engine fetches 2 stats for the container to be able to calculate percentages. See moby/moby#40437.
In general the best approach would be to use the newer https://github.com/moby/moby/blob/f472dda2e9dec18d4bf011fa6b2c5c388d6a20a7/client/container_stats.go#L32 as the percentages are calculated at the collector side (metricbeat).
Regarding the Podman error in fetching memory stats, we will investigate it to understand why this happens
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.
FYI until now I have made a local image from this branch code.
I have installed podman in my MacOs and created a local container
I have added the label:
podman container inspect 45394fdd9ab9
"Labels": {
"co.elastic.metricbeat.docker.forceContainerStatsStream": "true",
"maintainer": "NGINX Docker Maintainers <docker-maint@nginx.com>"
Still error persists :
{"log.level":"error","@timestamp":"2024-01-31T16:43:04.321+0200","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).fetch","file.name":"module/wrapper.go","file.line":256},"message":"Error fetching data for metricset docker.memory: No memory stats data available","service.name":"metricbeat","ecs.version":"1.6.0"}
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
// force container stats stream to receive also precpu_stats when using podman | ||
// docker returns precpu_stats with or without streaming, podman does only return precpu_stats when streaming | ||
// this can be done using the label co.elastic.metricbeat.docker.forceContainerStatsStream | ||
var stream bool |
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.
Because we set stream false by default I would suggest to
var stream bool | |
stream :=false |
// docker returns precpu_stats with or without streaming, podman does only return precpu_stats when streaming | ||
// this can be done using the label co.elastic.metricbeat.docker.forceContainerStatsStream | ||
var stream bool | ||
if container.Labels != nil { |
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 nil check is not needed as long as is checked in next step
So overall:
stream := false
if labelValue, ok := container.Labels["co.elastic.metricbeat.docker.forceContainerStatsStream"]; ok {
streamlabel, err := strconv.ParseBool(labelValue)
if err != nil {
stream = streamlabel
}
}
@ahbonsu , I set the enhancement for you Additionally while we are investigating the docker.memory: No memory stats data available", can you please also update the description of this PR with any screenshot or evidence for correct functionality of this PR? |
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
Setting back to draft for now. |
This pull request is now in conflicts. Could you fix it? 🙏
|
|
This pull request is now in conflicts. Could you fix it? 🙏
|
Proposed commit message
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Run any container with podman and use the docker plugin pointing to the podman socket like : hosts: ["unix:///run/user/1001/podman/podman.sock"]. There will be an error when using podman:
"log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).fetch","file.name":"module/wrapper.go","file.line":256},"message":"Error fetching data for metricset docker.memory:>
Use a label on the container: co.elastic.metricbeat.docker.forceContainerStatsStream=true
... and the memory stats are read successfully.
Related issues
I did not see any issues but there are a few posts stackoverflow, etc. which do point out to this "issue"
Use cases
This allows the user to flag their podman container and see the memory usage.
Screenshots
Logs
"log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).fetch","file.name":"module/wrapper.go","file.line":256},"message":"Error fetching data for metricset docker.memory:>