newrelic / nri-varnish

New Relic Varnish Integration
MIT License
3 stars 9 forks source link

Add support for non-default varnish instance names #32

Closed deancouch closed 2 years ago

deancouch commented 2 years ago

Is your feature request related to a problem? Please describe.

When running varnishd with the -n option, there is no way to collect metrics. (varnishstat uses the same option)

man varnishd
...
       -n name
              Specify the name for this instance.  This name is used to construct the name of the directory in which varnishd keeps temporary files and persistent state. If the specified name begins
              with a forward slash, it is interpreted as the absolute path to the directory.

The resulting error when varnishstat does not use the matching name:

time="2021-08-11T15:57:41+10:00" level=error msg="Integration command failed" error="exit status 1" instance=myinstance integration=com.newrelic.varnish prefix=config/varnish stderr="[DEBUG] Error running varnishstat command: exit status 1\n[ERR] Error collecting metrics: exit status 1\n" working-dir=/var/db/newrelic-infra/newrelic-integrations

Feature Description

Allow the name of the varnish instance to be specified via configuration, and passed to varnishstat.

func CollectMetrics(systemEntity *integration.Entity, i *integration.Integration, VarnishName string) error {
    var argList = []string{"-j"}

    if VarnishName != "" {
        argList = append(argList, "-n", VarnishName)
    }

    statOutput, err := ExecCommand("varnishstat", argList...).Output()
    if err != nil {
        log.Debug("Error running varnishstat command: %s", err.Error())
        return err
    }

Describe Alternatives

Alternatives may include attempting to determine the name of the running varnish, however this is probably undesirable.

Additional context

man varnishstat
...
       -n <dir>
              Specify the varnishd working directory (also known as instance name) to get logs from. If -n is not specified, the host name is used.

The default location will be something like /var/lib/varnish/<hostname> Should be applicable to all varnish versions.

Priority

Must Have :)

Unless I've completely missed something and there is already a way to do this, I would be happy to submit a PR, thanks.

alvarocabanas commented 2 years ago

Hello @deancouch there is not a way to do this right now and we would really appreciate if you submit a PR to add this feature, thanks.