Arvados

15317-metrics @ 6d868844fae9f52455b2e1954382bf9e4bb66c22 -- developer-run-tests: #4060

• All agreed upon points are implemented / addressed.
  • Adds metrics for the limiting backend speed for file content retrieval, as viewed by keep-web, bucketed by file size (see note above).
  • I expect the forthcoming streaming/prefetch improvements we've been working on (#2960, #18961) will show up on the new "limiting backend speed" metrics.
  • Adds overall upload/download speed metrics bucketed by file size, which may help us focus subsequent investigations/optimizations based on real-world performance
  • Adds upload sync delay metric (not bucketed by file size because it's expected to be a function of manifest size more than file size)
  • These aren't exactly the metrics mentioned in the description, which as discussed offline is ok.
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • I think we want to leave this issue open to add keepstore backend metrics like upload/download speed bucketed by block size. This should surely wait until #2960 is merged.
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • Automated tests ✅. Manual tests, see note above.
• Documentation has been updated.
  • New prometheus metrics have help text.
• Behaves appropriately at the intended scale (describe intended scale).
  • Shouldn't affect scale. Tracking code adds some minimal overhead per chunk uploaded/downloaded.
• Considered backwards and forwards compatibility issues between client and server.
  • N/A
• Follows our coding standards and GUI style guidelines.
  • ✅

• Spitballing the name, what do you think about "apparent" or "relative," like:

  Name: "download_apparent_backend_speed",
  Help: "Apparent download speed from the backend (bytes per second) when serving file downloads to the client. Low values suggest client download speeds could be improved if keep-web could download data from the backend faster."

  
  I'm still not in love with it. The main thing I'm trying to get at is to more explicitly hint at the caveat in your comments, that the number can be affected by client transfer speed too.
  I kind of wonder if the fact that this is hard to describe suggests that this isn't the most helpful metric to report. I understand it gives some useful data, but that data is hard to interpret: you need additional information (which we apparently don't track) to understand whether or not the number is meaningful. Is there a clearer way we could present this?
• In sizeRange, should the comparisons be < rather than <=? Honest question, I don't know the answer, I think it kind of depends on reporting conventions in both Prometheus and our own metrics.
• Nit, no blank line after the func newDownloadTracker defintion, I don't know if we have a style rule about this but it's at least unusual.
• Nit, the bottom of TestMetrics has the comment // Dump entire metrics output in test logs written twice. It feels like it should be there at most once, and I wonder if you could avoid the need for it entirely by using a more descriptive variable name than buf. I also wonder whether we have any conventions about whether to log data like this before running checks on it, or afterwards.

Brett Smith wrote in #note-25:

• Spitballing the name, what do you think about "apparent" or "relative," like:[...]

I like "apparent" better than limiting, so I changed to that.

Is there a clearer way we could present this?

I've updated the help text to link to #note-25 above, which should at least discourage from trying to interpret the data just based on the metric name/help.

• In sizeRange, should the comparisons be < rather than <=? Honest question, I don't know the answer, I think it kind of depends on reporting conventions in both Prometheus and our own metrics.

Yes, I think it makes more sense to have an exactly 1M file counted in the 1M category. Fixed.

• Nit, no blank line after the func newDownloadTracker defintion, I don't know if we have a style rule about this but it's at least unusual.

Oops, gofmt usually fixes things like this, but it allows either 0 or 1 blank lines between funcs, so this is mismatched for no reason. Fixed.

• Nit, the bottom of TestMetrics has the comment // Dump entire metrics output in test logs written twice. It feels like it should be there at most once, and I wonder if you could avoid the need for it entirely by using a more descriptive variable name than buf. I also wonder whether we have any conventions about whether to log data like this before running checks on it, or afterwards.

Improved variable name and deleted comments.

15317-metrics @ ae7a179649bbe19761c49de65b10127592ef646b -- developer-run-tests: #4072

Tom Clegg wrote in #note-26:

15317-metrics @ ae7a179649bbe19761c49de65b10127592ef646b -- developer-run-tests: #4072

This basically looks good to me except:

I've updated the help text to link to #note-25 above

-1 to this. That comment was never intended to be documentation, and the information people are looking for is like one or two sentences in one bullet in the middle of a larger conversation with a lot of irrelevant stuff. Expecting anyone to read it seems user-hostile.

What if we started expanding doc/admin/metrics.html.textile.liquid? We could have a section for each service with a /metrics endpoint, and then a subsection to document each metric in more detail. I wouldn't expect this branch to backfill that whole thing, just start enough structure to document this endpoint, and establish the pattern for future documentation.

Failing that: #note-15 would be a better link. It is mostly about this and goes into more detail about how to interpret the number, with examples. Maybe go back and edit it to reflect the new name?

Thanks.

Oops, I meant to link to #note-15, note #note-25, but much of that criticism applies anyway. Updated to link to WebDAV performance metrics instead, and put a slightly updated version of the note-15 explanation there.