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

Add support for multiple datasources #40

Closed
wants to merge 2 commits into from
Closed

Conversation

v-zhuravlev
Copy link

Add support for multiple datasources.
prometheus and loki.

If single templated datasource found:

  • Should be named $datasource with label "Data Source"

If two templated datasources found:

  • Prometheus should be named $prometheus_datasource with label "Prometheus Data Source"
  • Loki should be named $loki_datasource with label "Loki Data Source"

Add support for multiple datasources
prometheus and loki.
Prometheus should be named $prometheus_datasource with label "Prometheus Data Source"
Loki should be named $loki_datasource with label "Loki Data Source"
@v-zhuravlev v-zhuravlev requested a review from rgeyer April 8, 2022 14:13
Copy link
Collaborator

@rgeyer rgeyer left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. I added a few comments to help clarify the intent of these rules.

Overall, great contribution, thank you!

Comment on lines +26 to +31
if template.Query != Prometheus && template.Query != Loki {
return Result{
Severity: Error,
Message: fmt.Sprintf("Dashboard '%s' templated datasource variable query is '%s', should be 'prometheus' or 'loki'", d.Title, template.Query),
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

While the dashboard-linter is currently very focused on prometheus (and increasingly, loki) datasources, we should only raise errors for things that we can check, and for which an opinionated best practice exists.

In this case, it's perfectly valid to have a dashboard with a primary (or single) datasource which is neither prometheus nor loki. The dashboard-linter simply has no opinion or rule for those dashboards.

Comment on lines +37 to +47
// move loki to 0 index
sort.SliceStable(datasources, func(i, j int) bool {
return datasources[i].Query < datasources[j].Query
})

if !(datasources[0].Query == Loki && datasources[1].Query == Prometheus) {
return Result{
Severity: Error,
Message: fmt.Sprintf("Dashboard '%s' with 2 templated datasources should have 'prometheus' and 'loki' types", d.Title),
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to the previous comment. It's "okay" if a dashboard has two (or more) datasources which are not loki and prometheus. The dashboard linter would simply have no opinion/rule in this case.

}
}

for _, template := range datasources {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The checks here are the opinion/rule that we want to enforce. Namely, when multiple datasources do exist, they are named with their query/type. 👍

Comment on lines +79 to +83
if len(datasources) == 1 {
return d.checkPrometheusOrLokiDS(&datasources[0])
} else if len(datasources) == 2 {
return d.checkPrometheusAndLokiDS(datasources)
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be a switch, but also, per the other comments, we only care if there is one datasource template, or more than one.

You could probably combine both of these functions into a single checkDatasourceTemplates or similar, which branches depending on len(...) > 1.

return Result{
Severity: Error,
Message: fmt.Sprintf("Dashboard '%s' templated datasource variable labeled '%s', should be labeled 'Data Source'", d.Title, template.Label),
Message: fmt.Sprintf("Dashboard '%s' has %d templated datasources, should be 1 or 2", d.Title, len(datasources)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any number greater than 0 is what we're looking for.

if datasource != "$datasource" && datasource != "${datasource}" {
return []string{"$datasource"}
}
} else if datasource_count == 2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be > 1 and I'd advise looping over the datasources, creating a slice containing $<type>_datasource and ${<type>_datasource} then looping over them again to see if the actual name is contained in the slice.

@CLAassistant
Copy link

CLAassistant commented Jun 15, 2022

CLA assistant check
All committers have signed the CLA.

@kovaxur
Copy link

kovaxur commented Oct 20, 2022

Hi,
Wouldn't it make sense to only check, if a templated datasource is used? I created a PR just to check for that:
#101

@v-zhuravlev
Copy link
Author

Closed in favor of #124

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.

None yet

4 participants