search

newrelic / nri-varnish

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

Add support for V1 varnishstat output #30

Closed driskell closed 2 years ago

driskell commented 3 years ago

Currently, if you are using nri-varnish with a Varnish 6 instance it does not record any metrics whatsoever.

This is because varnishstat has changed format. This PR implemented support for the new version key so that it can detect the old varnishstat output as well as the new version 1 varnishstat output.

I also added a test to check the format processed successfully.

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

driskell commented 3 years ago

I do not have access to snyk to see the failure.

gsanchezgavier commented 3 years ago

@driskell thanks for the contribution! snyk is ok , we move the package manager and the failing snyk is pointing to the old vendor.json. Let us prioritize this so we can allocate some time to review it thoughtfully.

driskell commented 3 years ago

@driskell thanks for the contribution! snyk is ok , we move the package manager and the failing snyk is pointing to the old vendor.json. Let us prioritize this so we can allocate some time to review it thoughtfully.

Thanks! Let me know if you need anything. Hopefully I got the right test coverage.

driskell commented 2 years ago

@gsanchezgavier Do you know if this might be looked at soon? Thanks.

gsanchezgavier commented 2 years ago

Hi @driskell sorry for the delay on the response, this is in our prioritized backlog so we should work on it soon I think.

gsanchezgavier commented 2 years ago

Hi @driskell do you mind if I jump into this PR adding some commits , I'm working on adding integration tests for this.

driskell commented 2 years ago

Hi @driskell do you mind if I jump into this PR adding some commits , I'm working on adding integration tests for this.

I don’t mind at all. Happy for you to do so.

gsanchezgavier commented 2 years ago

Thanks @driskell for your contribution!!

driskell commented 2 years ago

Thanks for merging and finishing the tests! I won’t be able to test this build for a few weeks but if not released by time I get back to work on it I will install it somewhere to test and let you know.