Skip to content
This repository was archived by the owner on Feb 16, 2026. It is now read-only.

varnishncsa: Add hitmiss hitpass indicators to Varnish:handling #4217

Merged
nigoroll merged 1 commit into from
Feb 19, 2025
Merged

varnishncsa: Add hitmiss hitpass indicators to Varnish:handling #4217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions bin/varnishncsa/varnishncsa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,14 @@ dispatch_f(struct VSL_data *vsl, struct VSL_transaction * const pt[],
case SLT_RespUnset:
process_hdr(FMTPOL_RESP, &CTX.watch_resphdr, b, e, 1);
break;
case SLT_HitMiss:
CTX.hitmiss = "miss";
CTX.handling = "hitmiss";
break;
case SLT_HitPass:
CTX.hitmiss = "miss";
CTX.handling = "hitpass";
break;
case SLT_VCL_call:
if (!strcasecmp(b, "recv")) {
CTX.recv_compl = 1;
Expand All @@ -1136,10 +1144,10 @@ dispatch_f(struct VSL_data *vsl, struct VSL_transaction * const pt[],
} else if (!strcasecmp(b, "hit")) {
CTX.hitmiss = "hit";
CTX.handling = "hit";
} 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";
Comment on lines -1139 to 1152
Copy link
Member

@dridi dridi Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nigoroll nigoroll Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@dridi dridi Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@nigoroll nigoroll Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nigoroll nigoroll Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@nigoroll nigoroll Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bugwash opinion: extend %handling, as we are going to add breaking changes with #4213 this should be a good combination.
@dridi is this ok with you? I could also see if we can get purge and reset also...

Copy link
Member

@dridi dridi Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wfm

} else if (!strcasecmp(b, "synth")) {
Expand Down
104 changes: 104 additions & 0 deletions bin/varnishtest/tests/u00002.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
varnishtest "varnishncsa handling"

server s1 -repeat 4 {
rxreq
txresp
} -start

varnish v1 -vcl+backend {
backend none none;

sub vcl_backend_fetch {
# XXX would like to do everything in v_b_e, but
# return(pass(x)) is not supported
if (bereq.url == "/hitpass") {
set bereq.backend = s1;
} else {
set bereq.backend = none;
}
}

sub vcl_backend_response {
set beresp.ttl = 1y;
return (pass(1y));
}

sub vcl_backend_error {
set beresp.status = 200;
set beresp.ttl = 1y;
set beresp.body = bereq.url;
if (bereq.url ~ "^/hitmiss") {
set beresp.uncacheable = true;
}
return (deliver);
}

sub vcl_miss {
if (req.url == "/hitmisspass" && req.is_hitmiss) {
return (pass);
}
}

sub vcl_recv {
if (req.url == "/pass") {
return (pass);
}
if (req.url == "/synth") {
return (synth(204));
}
}
} -start

client c1 {
# prime
txreq -url "/hitmiss"
rxresp
txreq -url "/hitmisspass"
rxresp
expect resp.status == 200
txreq -url "/hitpass"
rxresp
expect resp.status == 200
txreq -url "/hit"
rxresp
expect resp.status == 200

# re-check
txreq -url "/hitmiss"
rxresp
expect resp.status == 200
txreq -url "/hitmisspass"
rxresp
expect resp.status == 200
txreq -url "/hitpass"
rxresp
expect resp.status == 200
txreq -url "/hit"
rxresp
expect resp.status == 200

# others
txreq -url "/pass"
rxresp
expect resp.status == 200
txreq -url "/synth"
rxresp
expect resp.status == 204
} -run

shell {
cat <<EOF >expect
miss /hitmiss
miss /hitmisspass
miss /hitpass
miss /hit
hitmiss /hitmiss
pass /hitmisspass
hitpass /hitpass
hit /hit
pass /pass
synth /synth
EOF
varnishncsa -d -n ${v1_name} -F "%{Varnish:handling}x %U" >have
diff -u expect have
}
7 changes: 7 additions & 0 deletions doc/changes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ Varnish Cache NEXT (2025-03-15)
.. PLEASE keep this roughly in commit order as shown by git-log / tig
(new to old)

* The ``hitmiss`` and ``hitpass`` handling indicators have been added to the
``Varnish:handling`` format of ``varnishncsa``.

* The scope of VCL variables `req.is_hitmiss` and `req.is_hitpass` is now restricted
to `vcl_miss, vcl_deliver, vcl_pass, vcl_synth` and `vcl_pass, vcl_deliver, vcl_synth`
respectively.

* Two fields have been added to the VMOD data registered with varnish-cache:

- ``vcs`` for Version Control System is intended as an identifier from the
Expand Down
6 changes: 3 additions & 3 deletions doc/sphinx/reference/varnishncsa.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,9 @@ Supported formatters are:
misses. In backend mode, this field is blank.

Varnish:handling
In client mode, one of the 'hit', 'miss', 'pass', 'pipe' or 'synth' strings
indicating how the request was handled. In backend mode, this field is
blank.
In client mode, one of the 'hit', 'hitmiss', 'hitpass', 'miss', 'pass',
'pipe' or 'synth' strings indicating how the request was handled. In
backend mode, this field is blank.

Varnish:side
Backend or client side. One of two values, 'b' or 'c', depending
Expand Down