varnishncsa: Add hitmiss hitpass indicators to Varnish:handling #4217
Conversation
| } else if (!strcasecmp(b, "miss")) { | ||
| } else if (!strcasecmp(b, "miss") && strcmp(CTX.handling, "hitmiss")) { | ||
| CTX.hitmiss = "miss"; | ||
| CTX.handling = "miss"; | ||
| } else if (!strcasecmp(b, "pass")) { | ||
| } else if (!strcasecmp(b, "pass") && strcmp(CTX.handling, "hitpass")) { | ||
| CTX.hitmiss = "miss"; | ||
| CTX.handling = "pass"; |
There was a problem hiding this comment.
Should we conditionally update only the hitmiss in these blocks?
In both vcl_hit and vcl_miss we can degrade to a pass, so technically hitmiss should be preserved (unless "-") while the handling should be unconditionally updated.
There was a problem hiding this comment.
I do not understand what you are trying to say. The intention is to preserve hitmiss and hitpass when we see a call MISS or call PASS. When hitmiss turns into a pass, the value ends up as pass. Is this what you are trying to say?
There was a problem hiding this comment.
Note: using simplified formats %hitmiss and %handling to distinguish with hitmiss and hitpass values to hopefully avoid ambiguity.
What I had in mind was "%hitmiss/%handling" leading to for example "hitmiss/pass" after a return(pass) from vcl_miss in a hit-for-miss scenario.
Reading the current documentation for %hitmiss and %handling formats I think we should actually not change them this way because %hitmiss is intentionally limited and I think %handling is not a good host for "hitmiss" and "hitpass" values.
What I mean by that is that a hit-for-pass transaction is handled the same as a regular pass transaction, whether you return(pass) from vcl_recv, vcl_hit, or vcl_miss. You essentially get a private fetch, this is how the request is handled. This is how I think %handling should be reported (and already is if I'm reading correctly).
We should maybe leave %hitmiss and %handling formats alone and introduce a new format with a finer-grained set of values. Let's for example call it %lookup and it could have the following values:
- hit
- miss
- hitmiss
- pass (no effective lookup)
- hitpass
- pipe (no effective lookup)
- purge
- synth (no effective lookup)
- reset (client disconnected before a lookup-or-not decision was made)
And of course, I'm counting on the documentation to clarify what's what. This way you can keep %hitmiss as a source of de-facto hit ratio with hits vs everything else in your VSL sample.
edit: replaced one %hitpass occurrence with %handling
edit2: s/replace/replaced/ in first edit
There was a problem hiding this comment.
So you are proposing exactly the same plus purge plus reset, just as yet another different tag, which represents a superset of %handling which represents a superset of %hitmiss?
To clarify: 🟢 exists already 🟡 proposed by this PR 🔵 proposed by Dridi in addition
- 🟢
hit - 🟢
miss - 🟡
hitmiss - 🟢
pass(no effective lookup) - 🟡
hitpass - 🟢
pipe(no effective lookup) - 🔵
purge - 🟢
synth(no effective lookup) - 🔵
reset(client disconnected before a lookup-or-not decision was made)
I have not checked if reset can be implemented properly, at this point I would guess it might be equivalent to the existing -. So all in all, the proposal is to also add purge, fine.
And should we have a new %format? @gquintard says no, @dridi says yes, I don't care. But I do not think that %format inflation helps much here, I would just keep %handling.
There was a problem hiding this comment.
It seems like I forgot to reply to some comments:
What I had in mind was "%hitmiss/%handling" leading to for example "hitmiss/pass" after a return(pass) from vcl_miss in a hit-for-miss scenario.
Yes. How does this not work with the current patch? I added this to the test case: 143390b
The hitmiss rightly gets turned into a pass because of a deliberate vcl return.
What I mean by that is that a hit-for-pass transaction is handled the same as a regular pass transaction, whether you return(pass) from vcl_recv, vcl_hit, or vcl_miss. You essentially get a private fetch, this is how the request is handled. This is how I think %handling should be reported (and already is if I'm reading correctly).
😖 This patch proposes to report a difference between a regular pass and a hitpass. Before this patch, it was reported as pass, now as hitpass. That is intentional and I think it is helpful (unless we actually go for yet another %format).
nigoroll
force-pushed
the
varnishncsa_hitmiss_hitpass
branch
from
d2ab910
to
af7b3d9
Compare
The title should say it all. Simple straight forward extension of the handling strings.
@martin-uplex