Skip to content

Conversation

@sagebind
Copy link
Owner

@sagebind sagebind commented Sep 28, 2019

Initial implementation of #47. Metrics are collected at every progress callback and written to a set of shared fields that can be read back by accessing the Metrics object through ResponseExt.

@sagebind sagebind added the feature A new feature! label Sep 28, 2019
@sagebind
Copy link
Owner Author

sagebind commented Oct 2, 2019

Currently there's a bug somewhere in here where total_time() is returning things in second-resolution. For example, a request that takes 499ms returns 0, but a request that takes 501ms returns 1 second. While I would be surprised if this was a libcurl bug, I can't see anywhere else where the bug might be.

All the other timers are returning values accurate to at least 1ms.

@sagebind
Copy link
Owner Author

sagebind commented Oct 3, 2019

Found the issue with timing metric accuracy, it is an upstream compilation issue. Curl is falling back to the following internal implementation of Curl_now():

struct curltime Curl_now(void)
{
  /*
  ** time() returns the value of time in seconds since the Epoch.
  */
  struct curltime now;
  now.tv_sec = time(NULL);
  now.tv_usec = 0;
  return now;
}

This is why the metrics are so imprecise. I will create an upstream patch to fix this build configuration issue.

@sagebind
Copy link
Owner Author

sagebind commented Oct 3, 2019

Upstream PR: alexcrichton/curl-rust#305

@sagebind sagebind marked this pull request as ready for review October 13, 2019 03:59
@EliSnow
Copy link

EliSnow commented Oct 15, 2019

Precision down to 1ms is probably "good enough" but would it be possible to have even more precision?

@sagebind
Copy link
Owner Author

sagebind commented Oct 15, 2019

Technically yeah, curl 7.61.0 (relatively new) added new _T variants of existing time metrics that return longs instead of floats that are measured in microseconds. Using these new flags effectively locks us into requiring static-curl where we can control what curl version we have instead of using the system-wide version.

Unless we can do some sort of feature-detection at runtime... theoretically it would be possible.

Another option would be to switch between the old and new flags at compile time based on whether static-curl is enabled.

Also note that the precision available is also highly dependent on what system clocks curl is compiled to use (which we also can't control when using the system-wide curl).

@EliSnow
Copy link

EliSnow commented Oct 15, 2019

Compile time feature/checks with a static-curl sounds preferable to avoid any runtime overhead.

@sagebind
Copy link
Owner Author

sagebind commented Nov 5, 2019

The _T constants are not currently exposed by curl-sys. I'll leave that enhancement for a future PR to implement so that this one is no longer blocked. Improved precision will have no effect on the API, so we can ship the API now without it.

@sagebind
Copy link
Owner Author

sagebind commented Nov 8, 2019

There's a few things that curl actually doesn't track, like distinguishing between request upload time and response download time. Ideally we'd be able to provide a bit more info, like the fields available in the JavaScript timing API that look quite reasonable: https://developer.mozilla.org/en-US/docs/Web/API/Resource_Timing_API/Using_the_Resource_Timing_API. We should be able to augment curl's existing metrics by adding our own measurements in a few key callback functions, but I will leave this for a future PR.

For now, I left the API somewhat conservative to start with so we can get it into developer's hands to experiment with.

@sagebind sagebind merged commit 9909eda into master Nov 8, 2019
@sagebind sagebind deleted the stat branch November 8, 2019 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature A new feature!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants