fix: handle server-timing header merging #26
Conversation
|
Sorry @Nytelife26, I did a quick fix myself. I didn't notice that you already raised a pull request. When I look at the output, it seems there is some additional text at the end, divided by a comma. Hence I cut it at the comma and take the first element. Is your approach, first comma then equals sign, more robust perhaps? |
My logic here was that each entry of the header is divided by a comma (this will continue if more are added), so it should split by those first, then extract the value from the entry. I can't imagine the other way around would cause a problem. Both approaches will break if the order of entries changes anyway. The most technically correct solution would be to parse the entries, but that logic would be complex and introduce notable latencies in processing results. Ultimately, it's your call which you'd rather use, or if you want me to attempt to implement something less quick and dirty. |
Nytelife26
force-pushed
the
fix/header-merging
branch
from
a08cda1 to
f852078
Compare
Nytelife26
force-pushed
the
fix/header-merging
branch
from
f852078 to
89aa415
Compare
There was a problem hiding this comment.
The latest commit introduces the same system that Cloudflare uses for consistency and more robust handling.
| timing_match = TIMING_DURATION_RE.search(r.headers["Server-Timing"]) | ||
| coll.server.append(float(timing_match.group(1)) / 1e3) |
There was a problem hiding this comment.
In its current state, this change is fallible - much like before, but in a way that will crash instead of returning an erroneous value. We could handle this condition, but short of reporting None to invalidate the entire test, it would involve some compromise.
Fixes #25.
This should also make us immune to further silliness, in case they add anything else.