Add interval bandwidth reporting option #2
Conversation
|
What are you using this for, exactly? How big are the downloads you are doing since the time of a complete fetch is not enough? |
|
Using this to run flent against an http server which obtains large 2+ MB files. Time of a single fetch is typically in the tens of seconds, which is not granular enough |
tohojo
left a comment
tohojo
left a comment
There was a problem hiding this comment.
I'm fine with merging this, but please fix the issues outlined here. Also, please add a Signed-off-by tag to the commit, as per the kernel process guideline: http://ltsi.linuxfoundation.org/developers/signed-process :)
src/getter.c
Outdated
| speed = report_bytes_dl / time_delta; | ||
| fprintf(stderr, "GETTER_INTERIM_RESULT[%u]=%f\r\n", report_count, speed); | ||
| fprintf(stderr, "GETTER_INTERVAL[%u]=%f\r\n", report_count, time_delta); | ||
| fprintf(stderr, "GETTER_ENDING[%u]=%f\r\n", report_count, now); |
There was a problem hiding this comment.
This is going to be garbled if there is more than one active worker. I.e., you are going to get values from alternating values from different workers, which means that the interval you calculate here will be wrong, and the values will be misleading. You'll need to have a separate series for each worker, so do the interval calculation in the worker thread and pass that along with the value to the reporter thread, then add a worker ID to the output.
Also, I'm not particularly fond of netperf's output format for this. I'd prefer if you output each data point on a single line in a format that works for both human and machine consumption. Something like:
Worker 1: 400 bytes over 0.254 seconds ending at 1507885098.234
Or you can output the speed instead of bytes, of course. Don't have any strong opinions on that... And feel free to tweak the exact output format as well if you want; the most important is keeping each data point on a single line.
| if ((res = curl_easy_setopt(data->curl, CURLOPT_XFERINFODATA, data)) != CURLE_OK) { | ||
| fprintf(stderr, "cURL option error: %s\n", curl_easy_strerror(res)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Right now you're adding both report functions for new versions of curl. The old ones should probably be in an #else, no?
The XFERINFOFUNCTION option was added in curl 7.32 which was released more than 4 years ago, though, so feel free to just skip the compatibility stuff and only support that.
src/worker.c
Outdated
|
|
||
| /* Enable periodic worker reporting if enabled on CLI */ | ||
| if (data->worker_report_interval > 0) { | ||
| fprintf(stderr, "Enable reporting on write pipe fd: %d\n", data->pipe_w); |
There was a problem hiding this comment.
Looks like a leftover debug print?
src/worker.c
Outdated
| if ((res = curl_easy_setopt(data->curl, CURLOPT_NOPROGRESS, 0L)) != CURLE_OK) { | ||
| fprintf(stderr, "cURL option error: %s\n", curl_easy_strerror(res)); | ||
| } | ||
| data->last_dlnow = (curl_off_t)0; |
There was a problem hiding this comment.
Heh, think the compiler is smart enough to apply the right type to constants? :)
src/worker.c
Outdated
|
|
||
| static int reset_worker(struct worker_data *data) | ||
| { | ||
| fprintf(stderr, "RESET WORKER\n"); |
|
Thanks for the comments, will revise at the beginning of next week |
Signed-off-by: Eric Friedrich <efriedri@cisco.com>
|
Erm, your patch doesn't seem to work at all for me: |
2 participants
I'd like to be able to use flent to generate bandwidth graphs for http (much like netperf). This PR enhances http-getter to output netperf like interim bandwidth data from curl.