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

libcontainer/intelrdt: remove 'omitempty' property from CMT and MBM counters #2644

Merged
merged 1 commit into from Oct 16, 2020

Conversation

xiaochenshen
Copy link
Contributor

If the values of CMT and MBM counters are zero, they will be omitted as
empty items when getting Intel RDT stats.

Remove 'omitempty' property from CMT and MBM counters to display zero
values.

Signed-off-by: Xiaochen Shen xiaochen.shen@intel.com

…ounters

If the values of CMT and MBM counters are zero, they will be omitted as
empty items when getting Intel RDT stats.

Remove 'omitempty' property from CMT and MBM counters to display zero
values.

Signed-off-by: Xiaochen Shen <xiaochen.shen@intel.com>
@xiaochenshen
Copy link
Contributor Author

@Creatone @kolyshkin @AkihiroSuda
Could you help review this PR? Thank you!

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@xiaochenshen
Copy link
Contributor Author

@AkihiroSuda @crosbymichael
@Creatone
Could you help review at your convenience? This is only a trivial fix.

@AkihiroSuda AkihiroSuda merged commit d8bfd6c into opencontainers:master Oct 16, 2020
@eero-t
Copy link

eero-t commented Oct 16, 2020

Doesn't e.g. BlkIo have the same problem?

@xiaochenshen
Copy link
Contributor Author

Doesn't e.g. BlkIo have the same problem?

@eero-t From my understanding, it depends on the meaning of the value in the field. If the zero/nil/false value has special meaning and it should not be omitted, we'd better remove omitempty option. Please correct me if I am wrong.

From https://golang.org/pkg/encoding/json/:
The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

@eero-t
Copy link

eero-t commented Oct 16, 2020

@xiaochenshen For example in types/events.go all fields have "omitempty":

type BlkioEntry struct {
        Major uint64 `json:"major,omitempty"`
        Minor uint64 `json:"minor,omitempty"`
        Op    string `json:"op,omitempty"`
        Value uint64 `json:"value,omitempty"`
}

type Blkio struct {
        IoServiceBytesRecursive []BlkioEntry `json:"ioServiceBytesRecursive,omitempty"`
        IoServicedRecursive     []BlkioEntry `json:"ioServicedRecursive,omitempty"`
        IoQueuedRecursive       []BlkioEntry `json:"ioQueueRecursive,omitempty"`
        IoServiceTimeRecursive  []BlkioEntry `json:"ioServiceTimeRecursive,omitempty"`
        IoWaitTimeRecursive     []BlkioEntry `json:"ioWaitTimeRecursive,omitempty"`
        IoMergedRecursive       []BlkioEntry `json:"ioMergedRecursive,omitempty"`
        IoTimeRecursive         []BlkioEntry `json:"ioTimeRecursive,omitempty"`
        SectorsRecursive        []BlkioEntry `json:"sectorsRecursive,omitempty"`
}

But it's common for minor for a device file to be zero.

Minor being omitted for some of the data output by events looks IMHO pretty odd, and I can't see any advantage on it, it just makes parsing the data from "events" command harder.

(I noticed this when testing my own code for some other device limits, as I had modeled that code after BlkIo.)

@xiaochenshen
Copy link
Contributor Author

@eero-t
In my opinion, BlkIo has the same problem from your description.
Maybe you could open an issue or PR for further discussion?

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.

None yet

4 participants