Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backend: implement alerting query #847

Merged
merged 34 commits into from
Jan 13, 2020

Conversation

vignesh-reddy
Copy link
Contributor

Issue #801

Implemented alerting query issue to get data from Zabbix and parse the query by fetching groups, hosts, apps, and items.

@vignesh-reddy vignesh-reddy changed the title Alerting new historymetrics Backend: implement alerting query Dec 21, 2019
Copy link
Collaborator

@alexanderzobnin alexanderzobnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the first glance I see plugin doesn't return any data. I think it might be related to authentication. I'll try to figure it out.

pkg/models.go Outdated
RealHosts bool `json:"real_hosts,omitempty"`

// History GET
History int `json:"history,omitempty,string"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for history type 0 since integer 0 will be treated as an empty value. Use *int instead.

Suggested change
History int `json:"history,omitempty,string"`
History *int `json:"history,string,omitempty"`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderzobnin
Copy link
Collaborator

https://github.com/alexanderzobnin/grafana-zabbix/pull/847/files#diff-2cb344693fe3302e52c776fc8f6e3964R64
Datasource should be written in cache before returning:

newDs := b.newZabbixDatasource()
b.datasourceCache.Set(dsInfoHash, newDs)
return b.newZabbixDatasource()

if useTrend {
response, err = ds.ZabbixRequest(ctx, tsdbReq.GetDatasource(), "trend.get", params)
} else {
params.History = k
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

History should be a pointer in order to prevent it to be omitted.

Suggested change
params.History = k
params.History = &k

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appFilter := firstQuery.GetPath("application", "filter").MustString()
itemFilter := firstQuery.GetPath("item", "filter").MustString()

items, err := ds.getItems(ctx, tsdbReq.GetDatasource(), groupFilter, hostFilter, appFilter, itemFilter, "num")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getItems() return empty list in my case. Looks like something goes wrong there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're able to get items with specific queries. Could this be related to the Regex filter not working properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any regex was used then it would never return any items, with the latest change that should be resolved.

filteredItems := zabbix.Items{}
for _, item := range items {
if item.Status == "0" {
matched, err := regexp.MatchString(itemFilter, item.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This filter doesn't work. There're 2 cases in filters:

  1. Filter is a group/host/app/item name. So exact string comparison should be used.
  2. Filter is a regex (wrapped in /, like /CPU (.*) time/). Since go regex should be compiled without first and last /, it should be removed before MatchString() execution.

See https://github.com/alexanderzobnin/grafana-zabbix/blob/master/src/datasource-zabbix/zabbix/zabbix.js#L417

function findByFilter(list, filter) {
  if (utils.isRegex(filter)) {
    return filterByRegex(list, filter);
  } else {
    return findByName(list, filter);
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're working on the Regex filter now. I pointed this out earlier but we set it aside to get a PR out before the holidays. The trickiest bit right now is handling flags, which a few are currently supported but the syntax for that is different in Go.

Copy link
Contributor Author

@vignesh-reddy vignesh-reddy Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the Regex filter the following way:

  1. Check if the filter is a regex
  2. If it is a regex then check for any flags, separate them and covert them to go regex format
  3. If it is not regex do a regular string comparison

8bb98ae

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also b6c2116
Added unit tests and added a check for supported flags.

@alecxvs
Copy link
Contributor

alecxvs commented Jan 2, 2020

Caching new datasource instances: 74b97b6

@@ -429,14 +440,20 @@ func (ds *ZabbixDatasource) getHosts(ctx context.Context, dsInfo *datasource.Dat
if err != nil {
return nil, err
}
isRegex, hostFilter := parseFilter(hostFilter)
re := regexp.MustCompile(hostFilter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use MustCompile if the filter isn't meant to be a regex. If it's not a valid pattern, it will panic.

Also perhaps we want to use something that returns an error when it fails to compile so we can surface that to the user as a user error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, 04b1b2a

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b6c2116
Cleaned it up a bit and added unit tests for parseFilter

@alexanderzobnin alexanderzobnin merged commit 3f57197 into grafana:backend Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants