vmod_xkey: Add a n_xkey counter

[xordspar0]

This adds a counter for the number of xkeys.

Co-authored-by: Ben Zvan ben.zvan@target.com

[xordspar0]

In the future we plan to add a metric for the memory used by xkeys and possibly others.

[xordspar0]

Oops, we made a mistake. This increments n_xkey for every backend request, even if the xkey already exists from a previous request. Test case incoming.

[xordspar0]

☝️ That's fixed now.

[xordspar0]

It looks like the Travis builds run against Varnish 4.1. That won't work for this since counters were first exposed to VMODs in 6.0. That's the reason for the configure script failure: in Varnish 4.1, the VARNISH_COUNTERS autoconf macro wasn't defined yet.

[gquintard]

Moving to circleci is on my plate, but with a low priority. I would warmly welcome a PR to setup circleci, or to update the Travis file, if you're up for it.

On Thu, Feb 27, 2020, 15:24 Jordan Christiansen notifications@github.com wrote:

It looks like the Travis builds run against Varnish 4.1. That won't work for this since counters were first exposed to VMODs in 6.0. That's the reason for the configure script failure: in Varnish 4.1, the VARNISH_COUNTERS autoconf macro wasn't defined yet.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/varnish/varnish-modules/pull/151?email_source=notifications&email_token=AA42AKKBAHXI5E4HDKNUMRTRFBDURA5CNFSM4K47UXS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENGLEYA#issuecomment-592228960, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA42AKLNCCVEWNOMT3M4HETRFBDURANCNFSM4K47UXSQ .

[benzvan]

I just realized that makes any release of varnish-modules after this merge commit no longer backward compatible with varnish < 6.0. Probably fine though.

[gquintard]

totally fine, there's a varnish-module branch for each varnish-cache branch

[xordspar0]

I don't have any experience with CircleCI, but I can definitely change the Travis config to point to a different version of Varnish.

[dridi]

The master branch should probably test the weeklies.

[xordspar0]

It looks like vmod-bodyaccess has some build failures. It also has build failures master, but these failures are different. Is this related to the Varnish upgrade and should I fix them in this branch?

[gquintard]

a few things have (unexpectedly) changed in varnish, and bodyaccess needs to be updated. I'll try to have a look next week (unless you beat me to it, but in another branch), then we'll have a clear Travis view here

[gquintard]

@xordspar0, I've just pushed the body_access fixes, so you should be able to rebase without the .travis.yml commits and it should be fine.

Small bikeshedding: can you name the counter g_xkey_keys please? the g_ will make it clear it's a gauge, and having a xkey namespace will make additions easier to name in the future.

[dridi]

Small bikeshedding: can you name the counter g_xkey_keys please? the g_ will make it clear it's a gauge, and having a xkey namespace will make additions easier to name in the future.

I disagree with adding one more layer of namespacing, the gauge is already in the XKEY. namespace so really we should change it to XKEY.g_keys.

What other kind of keys do you think vmod_xkey would keep track of?

[gquintard]

The ever-watching Dridi is right, I didn't realize the counter was already in its own subsection. The gauge point still holds, so XKEY.g_keys is the way to go here.

[gquintard]

@xordspar0, I'm not sure you need that pointer, but it looks like we are missing the vsc file in EXTRA_DIST to make travis happy

[dridi]

My bad: https://github.com/varnishcache/varnish-cache/commit/8c91e49d1a516d86a1b84ac990c83c8d8cb3aa77

[xordspar0]

Thanks! I was struggling to reproduce this locally since something weird is going on with pkg-config for my local builds.

[xordspar0]

Alright, the build succeeds, the tests pass, everything seems to be working! Do you need me to squash or rebase my branch?

[gquintard]

I only added a couple of comments (style nitpick and the assert point). All good for me, but I'd like an extra amen from @Dridi or @nigoroll

[gquintard]

I'll merge at the end of the week if I don't hear any complaints

[xordspar0]

Ok, that should resolve all comments so far.

[gquintard]

rebase, squash, and we should be good :-)

[xordspar0]

Done.

[gquintard]

I'm merging, the rest, if any, can be handled later.

Thank you for this PR!