sensu / sensu-influxdb-handler

Sensu Go InfluxDB Metrics Handler
https://sensu.io
MIT License
14 stars 16 forks source link

Feature Suggestion: Add subscriptions as tags #9

Open timbutler opened 5 years ago

timbutler commented 5 years ago

Just started logging some of the metrics to Influx, but found one minor part missing. When using subscriptions to group items (eg part of "web" and also part of "production"), adding additional tags to the Influx data means this can be used for filtering. This way, data can be shown for all "web" systems and/or also all "production".

Since Influx doesn't easily allow multiple values for the same tag, I've taken a slightly different approach with a fork of the code here: https://github.com/timbutler/sensu-influxdb-handler/commit/f66796dab1982ac9b32b8ac2555bcfad8166f627

However, there's probably a neater way of handling this but still allowing easy grouping. Is there a better approach and has anyone else raised this as a request?

nikkictl commented 5 years ago

Hey @timbutler, you bring up an excellent point! I'm sure many others would like the ability to add check subscriptions as tags. I do want to point out that not every event being handled is guaranteed to contain a check, for example events created by Sensu's statsd server. At the very least, we would want to verify the event contains a check before assuming its existence. I would also suggest changing the tags to a more traditional key/value set, and perhaps even allowing the user to pass a tag-subscriptions flag:

if tagSubscriptions {
    if event.Check == nil {
        return fmt.Errorf("event does not contain a check, cannot add subscription tags")
    } 
    for _, sub := range event.Check.Subscriptions {
        tags["sensu_subscription"] = sub
    }
}

There's a lot of room for improvement here! We initially only covered the base use case, but I'd love to see the functionality you're describing, including some other customization such as a precision flag.

timbutler commented 5 years ago

Hi @nikkiki, thanks for the response!

The issue with the basic K/V set (unless I'm overlooking something?) is that Influx don't allow the same key for tags per metric and the example above would also only pass the last entry.

I also thought about simply sending the subscription slice as a simple delimited string (eg tags["subscriptions"] = strings.Join(event.Check.Subscriptions, ":") but search then of course becomes less concise if there's overlap with subscription names.

It's possible that I've overthought the answer however, appreciate the work on the handler so far and will publish some checks (which return Influx metrics) over the next week as well to suit.

nikkictl commented 5 years ago

@timbutler Oh okay, I see what you're saying. What about the following example? I'm not an expert at Influx Query Language, but it seems like you could query based on substrings of the key (ex. does it contain 'sensu_subscription'). I'm just trying to ensure consistency and context (ex. sensu_subscription_0/linux provides more context than sub_linux/1), so if this proposal doesn't work for your use case, let's keep thinking through some!

for i, sub := range event.Check.Subscriptions {
        tags["sensu_subscription_"+i] = sub
}
timbutler commented 5 years ago

Hi @nikkiki, I had it initially like this, but then ran into the issue of the order of the subscriptions. For example, if I had "web, production, us-east-1" and the 2nd entity had `"us-east-1, "web, production", being able to group by and/or filter on subscription wouldn't quite work as I can't use wildcards for the tag fields (as far as I know!).

Here's an example of the query I'm using to filter: SELECT mean("rttms") FROM "ping" WHERE ("sensu_entity_id" != '<ENTITYNAME>' AND "sub_us-east-1" = '1') AND $timeFilter GROUP BY time($__interval), "sensu_entity_id" fill(none)

This allows me to select all checks from within us-east-1 to display within Grafana or similar. I can also filter out particular groups of servers as well, ie if I need to only display the web servers.

Note: I have to filter out the entity by name as well as I'm using the agents more as proxies for remote checks, otherwise the inclusion would be fine.

I'm sure there's probably a neater method than my quick fix, ideally I'd like the way to dynamically create filters (eg, so I could select via "dc=us-east-1" and use dc to create the list of DC's) but so far subscriptions has been the easiest way. Hopefully someone with deeper Sensu / Influx integration has some thoughts on how to better achieve this!

Thanks for your input so far, it's been greatly appreciated.

marcseguin commented 5 years ago

Hi there, I'm still pretty unexperienced here so please forgive me if I make mistakes.

If I understand correctly than the current behaviour is that tagging is limited to the influxdb tag "sensu_entity_name" (which maps the the sensu attribute "event.Entity.Name" and you exchange here for adding a tag for the the sensu attribute "event.Check.Subscription". Both would be hardcoded, right?

Wouldn't it be possible (and good approach) that an adminstrator himself creates and maintains the mapping as he needs.

nikkictl commented 5 years ago

Correct, in that scenario both would be hardcoded. @marcseguin I totally agree, it's probably best that we keep the mapping as generic as possible so administrators can customize their tags. I think it's worth bringing up the sensu-go-metric-tag-enrichment mutator that @calebhailey hacked together for this use case. Using a mutator to map and inject metric tags for check fields, entity fields, and labels is definitely the more Sensu way.

calebhailey commented 5 years ago

There's also this idea: https://github.com/sensu/sensu-go/issues/2160