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 support for Windows specific metrics in Stats #663

Merged
merged 2 commits into from
Jul 28, 2017
Merged

Add support for Windows specific metrics in Stats #663

merged 2 commits into from
Jul 28, 2017

Conversation

cstavro
Copy link

@cstavro cstavro commented Jul 25, 2017

Docker for Windows has some new metrics that are currently ignored.

This adds support for reading those values from the docker response with no attempt to decode their meaning as that is an exercise left to the application.

Some details about the Windows specific implementations are here:

An example JSON blob returned from <windows_dockerd>/containers/{id}/stats :

{
    "read": "2017-07-25T14:29:14.9514732Z",
    "preread": "2017-07-25T14:29:13.9532443Z",
    "pids_stats": {},
    "blkio_stats": {
        "io_service_bytes_recursive": null,
        "io_serviced_recursive": null,
        "io_queue_recursive": null,
        "io_service_time_recursive": null,
        "io_wait_time_recursive": null,
        "io_merged_recursive": null,
        "io_time_recursive": null,
        "sectors_recursive": null
    },
    "num_procs": 8,
    "storage_stats": {
        "read_count_normalized": 5829,
        "read_size_bytes": 42184192,
        "write_count_normalized": 3474,
        "write_size_bytes": 23913472
    },
    "cpu_stats": {
        "cpu_usage": {
            "total_usage": 92803750,
            "usage_in_kernelmode": 0,
            "usage_in_usermode": 0
        },
        "throttling_data": {
            "periods": 0,
            "throttled_periods": 0,
            "throttled_time": 0
        }
    },
    "precpu_stats": {
        "cpu_usage": {
            "total_usage": 92795260,
            "usage_in_kernelmode": 0,
            "usage_in_usermode": 0
        },
        "throttling_data": {
            "periods": 0,
            "throttled_periods": 0,
            "throttled_time": 0
        }
    },
    "memory_stats": {
        "commitbytes": 1157246976,
        "commitpeakbytes": 1157246976,
        "privateworkingset": 145084416
    },
    "name": "/clever_cori",
    "id": "b8b0cb1c55f1285ae02bafcc2fc3f265b714716de5740c080909265cc98cc129",
    "networks": {
        "9983bec3-54d1-4152-96c0-3cc7ab3176d4": {
            "rx_bytes": 53086,
            "rx_packets": 259,
            "rx_errors": 0,
            "rx_dropped": 1,
            "tx_bytes": 46044,
            "tx_packets": 128,
            "tx_errors": 0,
            "tx_dropped": 0
        }
    }
}

@cstavro
Copy link
Author

cstavro commented Jul 25, 2017

I presume the test failure here is because the tests are running on a Linux system that doesn't have these extra fields in the Docker stats output. Does it make sense to have platform specific tests here? Or can we loosen up these failing tests so that they allow for this change?

@fsouza
Copy link
Owner

fsouza commented Jul 25, 2017

@cstavro appveyor runs windows, travis runs linux and macos. If this is a test that would work only on windows (or linux/macos), I'd recommend making it platform specific.

I haven't look at the source or the test failure yet (sorry about that), but I wanted to answer your question. I'll take a better look at the PR later today.

Thanks for contributing!

@cstavro
Copy link
Author

cstavro commented Jul 28, 2017

So it turns out the test failure was because I wasn't initializing the PreRead time.Time value in the test which seems to behave differently in 1.7.5 vs 1.8.x.

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@fsouza fsouza merged commit 3a44d1f into fsouza:master Jul 28, 2017
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