newrelic / nri-varnish

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

Varnish Plus attributes monitoring #37

Closed matewilk closed 2 years ago

matewilk commented 2 years ago

This PR adds some MSE_BOOK - book and MSE_STORE - store Varnish Plus properties.

All new properties are covered with tests.

It also has been tested with Varnish Plus in a (dev) environment and it works by sending appropriate metrics to New Relic.

The only issue (might not be related to this PR at all) is that the data is visible under NR Query Builder: query_builder_varnish_plus

But not under NR Data Explorer: (and yes, the timeframe was the same for both)

data_explorer_varnish_plus
paologallinaharbur commented 2 years ago

When adding new metrics we should remember to update the infraSpec to convert them to DM automatically. Writing it down not to forget

matewilk commented 2 years ago

Hey @paologallinaharbur

There is one more thing I'd like to add to the PR.

The customer who I implemented it for, wants to track also the following metrics from their Varnish Plus instance:

As you can see, the middle part of the property is specific to the customer.

My understanding of the codebase though, is that the middle part doesn't really matter, as it is sent to NR platform as a variable, as it is with for example MSE_BOOK.book1.g_bytes (that I've added in this PR). Please see the first screenshot from the PR description where the column name is BOOK or starts with book e.g BOOK.ALLOCINBYTES.

I'm guessing, we would have to give it a more generic name, plus there is the transformation from event to metrics that also needs to happen.

Have you got any suggestions in regards to that?

paologallinaharbur commented 2 years ago

In order to update the specs, there is an internal repository tracking them, ping me to get the URL. Without that as you noticed already all the data will stay as a sample.

Regarding the metrics changing name, it is a common issue when dealing with tags, since the key changes each time and we also need the value. If I understood correctly what you want to achieve it should not be an issue. However, converting them to DM is more complicated but still possible (I believe)

matewilk commented 2 years ago

@carlossscastro @paologallinaharbur

FYI I've added the properties I've mentioned in the previous post, thanks @paologallinaharbur for helping with this.

The PR is ready to be merged. Let me know if you find anything I need to modify.

Thanks!

kang-makes commented 2 years ago

Thank you for this contribution.

This is merged and released with the version 2.5.0