feat: reworked varnish_cache plugin #9432

rhajek · 2021-06-24T17:48:49Z

Added reworked version of the VarnishCache input plugin based on json parsing of varnishstat -j.
Conversion to metrics from Varnish stats is reworked, it solves issue with high cardinality when reloading Varnish server, parses backends into tags, supports "VBE.*" metrics.

Updated associated README.md.
Wrote appropriate unit tests.

ismaelpuerto · 2021-07-05T11:33:30Z

plugins/inputs/varnish_cache/varnish_cache.go

+
+// Find the most recent 'VBE.reload_' prefix using string compare
+// 'VBE.reload_20210623_170621_31083'
+func findActiveReloadPrefix(countersJSON map[string]interface{}) string {


This approach only work if you reload services, but if you load vcl manually with custom name doesn't valid

I reworked this PR, now it is part of the varnish plugin, varnishadm tool is used to get active vcl.

@ismaelpuerto is this concern resolved?

helenosheaa

Hi, @rhajek thanks for the PR! It's a significant change to the varnish plugin. I've left review comments with suggestions and some questions that would be great if you could respond to/ resolve.

Are we trying to support a specific version of varnish with this plugin or is it cross-version compatible?

There's also a test failure in circle-ci that would need to be resolved:

FAIL: TestJsonTypes (0.00s)
    varnish_test.go:617: 
        	Error Trace:	varnish_test.go:617
        	Error:      	Not equal: 
        	            	expected: float64(123.45)
        	            	actual  : <nil>(<nil>)
        	Test:       	TestJsonTypes
FAIL
FAIL	github.com/influxdata/telegraf/plugins/inputs/varnish

Has this been tested by someone who was experiencing the cardinality issue?

@ismaelpuerto have you had a chance to test this out and see if it works when you load a vcl manually? It would also be great if we could check that this is backwards compatible when the metric version is set to 1. As I would expect this change in behavior with removal of nonactive VCL's to only happen if a user configures the plugin to be metric version 2.

plugins/inputs/varnish/README.md

plugins/inputs/varnish/varnish.go

helenosheaa · 2021-08-31T21:53:44Z

plugins/inputs/varnish/varnish.go

@@ -139,6 +244,16 @@ func (s *Varnish) Gather(acc telegraf.Accumulator) error {
 			continue
 		}

+		//skip not active vcls
+		vMetric := parseMetricV2(stat)


Why is parse metrics v2 inside a function named process metrics v1? I would expect metric 1 to keep backwards compatibility for our users (so not removing nonactive VCL's).

My goal was also to fix https://github.com/influxdata/EAR/issues/2289 and strip the active VCL name from field name for v1 metrics. If we want v1 to stay fully backward compatible, we should revert this and remove also unnecessary varnishadm vcl.list check.

Hi,

The issue to keep nonactive VCL's is that it can generate huge of metrics that seems useless.

It can be very good to skip all inactive vcls.

There is a similar PR available on #6832 (but seems abandoned) related to #5622

I understand but we try to keep backwards compatibility as up until now nonactive vcl's were reported. The approach is to give users the option to opt in to new behavior rather than changing behavior on an upgrade of Telegraf.

plugins/inputs/varnish/README.md

plugins/inputs/varnish/varnish_test.go

plugins/inputs/varnish/varnish.go

plugins/inputs/varnish/varnish_test.go

plugins/inputs/varnish/varnish.go

plugins/inputs/varnish/README.md

powersj

Thanks for this MP. A couple of small in-line comments and questions and one higher-level question:

Can you explain why version 2 metrics include the id tag while version 1 metrics omit it? This seems to greatly increase the cardinality even on my little system running varnish + apache2 with the default configs. See my comment in the parseMetricV2 function.

Given one purpose of this MP is to reduce cardinality this seems to do the opposite as I have run it above.

Thanks!

edit: updated link to version 2 metrics

powersj · 2021-09-16T22:45:51Z

plugins/inputs/varnish/varnish.go

+	//check vcl.list output
+	if jsonOut[0].(float64) != 2 {
+		return "", fmt.Errorf("unsupported varnishadm format %v", jsonOut[0])
+	}


Are you aware of any docs on when and how this version value changes? Even though it is at 2 right now, it will be a breaking surprise when it does in fact change.

Not required, just something for next time: If it is relatively static, then it might have been nice to unmarshal this into an object, as you do below with vclStruct, to make the references below easier to follow.

The only documentation that I found:

https://varnish-cache.org/docs/6.6/reference/varnish-cli.html#json

https://varnish-cache.org/docs/6.6/reference/varnish-cli.html#vcl-list-j

version "2" is still the same from 6.0.3 released in 9 Feb 2019

May be, check for the presence of "vcl.list", "-j" is enough and we can ignore version.

Unmarshal json array like this

[ 2, ["vcl.list", "-j"], 1631878235.913, { "status": "active", "state": "auto", "temperature": "warm", "busy": 0, "name": "boot" } ]

into object is definitely better, I will try to fix this.

plugins/inputs/varnish/README.md

powersj · 2021-09-16T23:34:42Z

plugins/inputs/varnish/varnish.go

+				metric.fieldName = val
+			} else if val != "" {
+				metric.tags[sub] = val
+			}


This is where id is getting added as a tag and why I am asking at a high level why we are increasing the cardinality on that value. Is that something users want to index against?

It seems concerning to add anything that doesn't match _vcl or _field as a tag as any change to the data format is going to cause cardinality to increase.

Thanks for review. Id tag is added by the last default regexp rule (?P)

//generic metric like MSE_STORE.store-1-1.g_aio_running_bytes_write regexp.MustCompile(`([\w\-]*)\.(?P<id>[\w\-.]*)\.([\w\-]*)`),

My idea was not to increase the cardinality, but make the field names corresponding with https://varnish-cache.org/docs/trunk/reference/varnish-counters.html

Cardinality issue was caused because varnish adds timestamps into its metrics names when reloading. This should be solved by tripping vcl name from the name.

Example: we have following varnish metrics

"MEMPOOL.busyobj.allocs" "MEMPOOL.ssl_buf.allocs" "MEMPOOL.req0.allocs" "MEMPOOL.sess0.allocs" "MEMPOOL.req1.allocs" "MEMPOOL.sess1.allocs"

Middle part identifies the name of resource. There are limited number of these so it is suitable for tagging/indexing.
We reduce number of tags but increase number of fields. I think cardinality in this case is the same.

varnish,section=MEMPOOL,id=busyobj alloc=xxxx
varnish,section=MEMPOOL,id=ssl_buf alloc=xxxx
varnish,section=MEMPOOL,id=req0 alloc=xxxx

varnish,section=MEMPOOL busyobj.alloc=xxxx
varnish,section=MEMPOOL ssl_buf.alloc=xxxx
varnish,section=MEMPOOL req0.alloc=xxxx

Biggest disadvantage of this approach i see is that this approach will not be backward compatible with v1.
Varnish metrics can have various names, it tried to cover that in 'plugins/inputs/varnish/varnish_test.go:441'

@helenosheaa @ismaelpuerto @anthosz So should we remove the id tag and add middle part into field name?

If there's a static number of values that will be set as id=tagvalue I don't think it will be as much of cardinality problem as we were seeing with vcl name where fields included timestamps or ip's so they were constantly increasing.

However what is the advantage of splitting it out into a tag? The more consistency between v1 and v2 unless there is a value add I think the better.

That said I believe the build from this PR is being tested so shall we make a decision on this once we get feedback from that?

Splitting varnish metric names into tags can be useful when creating dashboards. Represent a newly added resource as a tag looks more natural to me, it will be automatically shown in the dashboard if you use generic queries and filters.

But, may be, more important is to keep v2 as much as possible backward compatible and not break existing things.
User can always use custom regexps in telegraf configuration to create custom tag splitting if needed.

telegraf-tiger · 2021-09-21T13:47:57Z

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

helenosheaa · 2021-09-21T21:26:19Z

hey @ismaelpuerto would you mind testing this build with the two different metric versions?

ismaelpuerto · 2021-09-21T22:24:24Z

Hello, I just test with both version and It looks fine, For this test I used varnish-plus-6.0.8r4 and I tested metrics like kvstore, vha and accounting, regarding this metric, maybe it make sense that exists a tag with value of <namespace>, but I can understand that this a special case with a low audience and right now, it's not priority.

Regarding tags, for me is fine have "section" only, I don't need the name of vcl as tag because we have metric "n_vcl" to know if there was loaded a new vcl.

rhajek · 2021-09-22T08:26:03Z

Hello, I just test with both version and It looks fine, For this test I used varnish-plus-6.0.8r4 and I tested metrics like kvstore, vha and accounting, regarding this metric, maybe it make sense that exists a tag with value of , but I can understand that this a special case with a low audience and right now, it's not priority.

Regarding tags, for me is fine have "section" only, I don't need the name of vcl as tag because we have metric "n_vcl" to know if there was loaded a new vcl.

To add a namespace tag for accounting metrics you can add following regexp into telegraf config. Should I include this rule to the defaults ?

[[inputs.varnish]]
regexps = ['^ACCG.(?P[\w-]).(?P<_field>[\w-.])']

helenosheaa · 2021-09-22T14:51:40Z

I think instead of adding it to defaults as it's potentially low audience right now, just add it as an example in the README so people can add if they want to.

ivankudibal · 2021-09-29T17:28:19Z

@popey - the approver is unclear

helenosheaa

@ivankudibal @rhajek lgtm, however we are just waiting for feedback from the customer on the change before we would merge this.

dg-nvm · 2021-11-05T10:32:34Z

@helenosheaa can we get artifacts rebuild again ? Without this I cannot test changes

powersj · 2021-11-05T18:54:45Z

@rhajek can you rebase on master and push, please? Artifacts are only available for 30-days and there are some folks who want to test this PR out. Updating the PR with a rebase should trigger the tests and package builds again.

If users feel comfortable building the go binary themselves they can also do the following:

git clone https://github.com/rhajek/telegraf -b varnish_cache
cd telegraf
make

The above will produce a binary for use on the system it was run on (i.e. if you build on linux/amd64, it will be a linux/amd64 binary).

dg-nvm · 2021-11-08T10:36:04Z

@powersj I had to upgrade Golang to do it but it worked and I will use my compiled one to test it

dg-nvm · 2021-11-08T11:16:38Z

@rhajek I still see plenty of reload_20211108_110316_28415 fields on new configuration:

varnishstat (varnish-6.0.6 revision 29a1a8243dbef3d973aec28dc90403188c1dc8e7)
Telegraf unknown (git: varnish_cache 40c4735)

[[inputs.varnish]]
  #use_sudo = false
  #binary = "/bin/varnishstat"
  stats = ["all"]
  #name = instanceName
#  fielddrop = ["reload_*"]

Do we have to add some special configuration to get rid of these multiple fields ?

rhajek · 2021-11-09T10:39:18Z

@rhajek I still see plenty of reload_20211108_110316_28415 fields on new configuration:

@dg-nvm thanks for testing this PR, you have to use metric_version = 2 in your telegraf.config to enable nonactive vcls removal.

telegraf-tiger · 2021-11-09T10:46:56Z

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1% increase)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here! 🐯

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_armv7.tar.gz
armhf.deb	i386.rpm	freebsd_i386.tar.gz
i386.deb	ppc64le.rpm	linux_amd64.tar.gz
mips.deb	s390x.rpm	linux_arm64.tar.gz
mipsel.deb	x86_64.rpm	linux_armel.tar.gz
ppc64el.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

anthosz · 2021-11-09T13:41:28Z

Hi @rhajek ,

Thank you very much for the works, I just tested with the artifacts of this day and seems to be good: I have only the active vcls.

In addition, I have one remark concerning the "unhealthy" field, I cannot find it via varnishadm/varnishstat and it's always set to zero (I tried to stop the backend -> instance sick on varnish side -> but unhealthy value is always set to 0)

anthosz · 2021-11-09T14:11:39Z

Another thing concerning custom_arguments (https://github.com/influxdata/telegraf/blob/db40a348a3dbefafdf20557d221522d3751d141d/plugins/inputs/varnish/README.md#custom-arguments):

It seems that the instance_name is not take into account when we use custom args: it need to be added manually
Once the instance name is set (manually), I have this error:
2021-11-09T14:05:34Z E! [inputs.varnish] Error in plugin: error gathering metrics: invalid character 'a' looking for beginning of value 2021-11-09T14:05:34Z E! [telegraf] Error running agent: input plugins recorded 1 errors
I have the feeling that it's due to vcl.list of varnishadm but not sure (debug mode doesn't help)
available auto cold 0 boot [...]

dg-nvm · 2021-11-09T16:39:09Z

@anthosz @rhajek I checked that too and yes, when using instance name command line looks like this:

/bin/varnishadm vcl.list -j -n /instance-name

Instead of:

/bin/varnishadm -n /instance-name vcl.list -j

Order of arguments matters, command and -j have to be at the end

rhajek · 2021-11-11T10:01:20Z

I fixed how to setup a custom instance name when using custom arguments in README.md.

telegraf-tiger · 2021-11-11T12:51:16Z

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1% increase)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them **here**! 🐯

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_armv7.tar.gz
armhf.deb	i386.rpm	freebsd_i386.tar.gz
i386.deb	ppc64le.rpm	linux_amd64.tar.gz
mips.deb	s390x.rpm	linux_arm64.tar.gz
mipsel.deb	x86_64.rpm	linux_armel.tar.gz
ppc64el.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

Co-authored-by: Helen Weller <38860767+helenosheaa@users.noreply.github.com>

telegraf-tiger · 2021-12-14T08:35:02Z

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here ! 🐯

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_armv7.tar.gz
armhf.deb	i386.rpm	freebsd_i386.tar.gz
i386.deb	ppc64le.rpm	linux_amd64.tar.gz
mips.deb	s390x.rpm	linux_arm64.tar.gz
mipsel.deb	x86_64.rpm	linux_armel.tar.gz
ppc64el.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

rhajek · 2021-12-14T08:59:43Z

@rhajek can you do a final rebase on master and resolve the conflicts on the readme? We can do a final review and land this if it looks good.

@powersj PR is rebased and ready for approval.

Co-authored-by: Helen Weller <38860767+helenosheaa@users.noreply.github.com>

anthosz · 2022-02-22T14:06:09Z

Dear @sspaink ,

Do you know when it will be released?

Best regards,

sspaink · 2022-02-22T14:14:17Z

It will be included in the next feature release which is scheduled March 16th, if you need it sooner I recommended using a nightly release artifact: https://github.com/influxdata/telegraf/blob/master/docs/NIGHTLIES.md

In the future, we are hoping to move to monthly releases so features should be released quicker: https://community.influxdata.com/t/request-for-community-feedback-rcs-monthy-releases/23703

anthosz · 2022-03-22T13:05:52Z

Hi @sspaink ,

I don't see any new release since the 16th (I prefer to use a "stable" version). Do you have an estimation?

Best regards,

powersj · 2022-03-22T13:22:09Z

Do you have an estimation?

Plan is to start the release tomorrow - we decided to hold off a little bit so we could land some other snmp changes.

sspaink · 2022-03-23T21:32:26Z

@anthosz v1.22.0 has been released: https://github.com/influxdata/telegraf/releases/tag/v1.22.0 !

anthosz · 2022-03-24T08:25:15Z

@sspaink thank you! \o/

anthosz · 2022-03-25T13:46:04Z

Good job to all for this PR, all works well :)

telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Jun 24, 2021

rhajek marked this pull request as draft June 25, 2021 06:22

rhajek marked this pull request as ready for review June 25, 2021 12:13

ismaelpuerto reviewed Jul 5, 2021

View reviewed changes

helenosheaa suggested changes Sep 1, 2021

View reviewed changes

rhajek requested a review from helenosheaa September 10, 2021 13:08

helenosheaa reviewed Sep 14, 2021

View reviewed changes

plugins/inputs/varnish/README.md Outdated Show resolved Hide resolved

sjwang90 removed the new plugin label Sep 16, 2021

powersj reviewed Sep 16, 2021

View reviewed changes

rhajek requested a review from helenosheaa September 27, 2021 09:07

helenosheaa approved these changes Sep 29, 2021

View reviewed changes

rhajek and others added 14 commits December 14, 2021 08:54

Apply suggestions from code review in README.md

ad53b38

Co-authored-by: Helen Weller <38860767+helenosheaa@users.noreply.github.com>

feat: varnish cache plugin reworked, updated README.md

11dd224

feat: varnish cache code cleanup, updated README.md

fd4ccc4

Update plugins/inputs/varnish/varnish.go

d9b4211

Co-authored-by: Helen Weller <38860767+helenosheaa@users.noreply.github.com>

Update plugins/inputs/varnish/varnish.go

af39131

Co-authored-by: Helen Weller <38860767+helenosheaa@users.noreply.github.com>

docs: added Varnish 6.0.2+ requirement to README.md

e200024

docs: added Varnish 6.0.2+ requirement to README.md

ea53c63

feat: varnish cache plugin reworked, simplify vcl.list json unmarshal

ea597c8

feat: varnish cache plugin reworked, remove id tag

8feeac9

feat: add accounting VMOD regexp example into README.md

384e032

feat: fixing S1017 staticcheck

06a7916

feat: fixing markdownlint in README.md

2e34c79

feat: updated custom arguments README.md

5f2ffcf

feat: rebased on master

c901824

rhajek force-pushed the varnish_cache branch from 64a8449 to c901824 Compare December 14, 2021 08:11

feat: rebased on master

b110ee0

sspaink approved these changes Dec 21, 2021

View reviewed changes

sspaink merged commit 6de4d34 into influxdata:master Dec 21, 2021

powersj pushed a commit to powersj/telegraf that referenced this pull request Jan 21, 2022

feat: reworked varnish_cache plugin (influxdata#9432)

64ecd5c

Co-authored-by: Helen Weller <38860767+helenosheaa@users.noreply.github.com>

sjwang90 mentioned this pull request Mar 5, 2022

[Varnish] Set tag with vcl active, for VBE.* and KVSTORE.* only retry metrics from vcl active #6832

Closed

3 tasks

anthosz mentioned this pull request Apr 27, 2022

varnish - VBE stats are not extracted if we use dynamic vmod #11040

Closed

feat: reworked varnish_cache plugin #9432

feat: reworked varnish_cache plugin #9432

Conversation

rhajek commented Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenosheaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

powersj left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhajek Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helenosheaa Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Sep 21, 2021

Artifact URLs

helenosheaa commented Sep 21, 2021

ismaelpuerto commented Sep 21, 2021

rhajek commented Sep 22, 2021

helenosheaa commented Sep 22, 2021

ivankudibal commented Sep 29, 2021

helenosheaa left a comment

Choose a reason for hiding this comment

dg-nvm commented Nov 5, 2021

powersj commented Nov 5, 2021

dg-nvm commented Nov 8, 2021

dg-nvm commented Nov 8, 2021

rhajek commented Nov 9, 2021

telegraf-tiger bot commented Nov 9, 2021

Artifact URLs

anthosz commented Nov 9, 2021 • edited Loading

anthosz commented Nov 9, 2021 • edited Loading

dg-nvm commented Nov 9, 2021

rhajek commented Nov 11, 2021

telegraf-tiger bot commented Nov 11, 2021

Artifact URLs

telegraf-tiger bot commented Dec 14, 2021

Artifact URLs

rhajek commented Dec 14, 2021

anthosz commented Feb 22, 2022

sspaink commented Feb 22, 2022

anthosz commented Mar 22, 2022

powersj commented Mar 22, 2022

sspaink commented Mar 23, 2022

anthosz commented Mar 24, 2022

anthosz commented Mar 25, 2022

rhajek commented Jun 24, 2021 •

edited

Loading

powersj left a comment •

edited

Loading

rhajek Sep 17, 2021 •

edited

Loading

helenosheaa Sep 17, 2021 •

edited

Loading

anthosz commented Nov 9, 2021 •

edited

Loading

anthosz commented Nov 9, 2021 •

edited

Loading