VictoriaMetrics / victoriametrics-datasource

Grafana datasource for VictoriaMetrics
GNU Affero General Public License v3.0
112 stars 14 forks source link

Two issues when we migrate from prometheus to victoriametrics datasource #205

Open chenlujjj opened 2 months ago

chenlujjj commented 2 months ago

Hi team, we're migrating the datasource from prometheus to victoriametrics, but find two issues:

before:

image

after:

image

Can you help to take a look into them? Thanks in advance

hagen1778 commented 1 month ago

@Loori-R could you please take a look? The first one is very annoying issue

Loori-R commented 1 month ago

Unfortunately, we are unable to prevent the query from being lost when switching the datasource in alert rules. This behavior is controlled by Grafana's internal code. When a datasource is switched, Grafana checks the type (which corresponds to the plugin ID that we cannot modify). If the type differs from the previous one, Grafana resets the query by invoking the newModel() function. You can find the relevant code in Grafana's repository here:

if (settings.type === previousSettings?.type) {
  return copyModel(item, settings);
}
return newModel(item, settings);

The newModel() function initializes a new query model without preserving the existing expression, which is stored in model.expr. Here is the newModel() function for context:

function newModel(item: AlertQuery, settings: DataSourceInstanceSettings): Omit<AlertQuery, 'datasource'> {
  return {
    refId: item.refId,
    relativeTimeRange: item.relativeTimeRange,
    queryType: '',
    datasourceUid: settings.uid,
    model: {
      refId: item.refId,
      hide: false,
      datasource: getDataSourceRef(settings),
    },
  };
}

As you can see, the model.expr field containing the query expression is not retained when creating a new model upon changing the datasource.

chenlujjj commented 1 month ago

@Loori-R Thank you.

we are unable to prevent the query from being lost when switching the datasource in alert rules.

Beside the alert rules, we find that things seem different for the dashboard panels or metric explorer. Before:

WeChatWorkScreenshot_25a27c46-91cd-4341-89df-d3c243027a80

After:

image

You can see that the query expression is retained, but not completely right. The metric name is put as the __name__ label and the label name containing dot is not handled correctly.

Loori-R commented 1 month ago

How it works:

The datasource uses the importFromAbstractQueries and exportToAbstractQueries methods to convert abstract queries into valid expressions and vice versa. When exporting from Prometheus using exportToAbstractQueries, the label names become distorted.

What we can do:

We can adjust our implementation to correctly handle labels with dots during import. This will preserve labels with dots when switching from VictoriaMetrics to Prometheus.

Why this isn't a complete solution:

This only resolves the issue in one direction. To fully address the problem, the exportToAbstractQueries method in the Prometheus datasource must correctly handle labels with dots in their names.

chenlujjj commented 1 month ago

@Loori-R Understood, thank you very much!

hagen1778 commented 1 month ago

This only resolves the issue in one direction. To fully address the problem, the exportToAbstractQueries method in the Prometheus datasource must correctly handle labels with dots in their names.

@Loori-R is this a complex task? Do you think you can submit a PR to grafana with fix?

dmitryk-dk commented 1 month ago

Hi @chenlujjj ! This feature was implemented in the new release. Please check it, and if you find any problem, please reopen the issue.

Loori-R commented 1 month ago

The following functionality has been implemented and included in the release: "set the default query type to instant when creating alerting rules"

However, I am reopening this issue because there is still a problem with dots in the labels when switching between VictoriaMetrics and Prometheus.

hagen1778 commented 1 month ago

Related change https://github.com/grafana/grafana/pull/95163