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

Fixed Timeshift and Moving Average on 0.0.5 #128

Merged
merged 4 commits into from
Mar 8, 2022

Conversation

drsoares
Copy link
Contributor

@drsoares drsoares commented Mar 9, 2021

Issue fix, #122. Inside TimeShift functions and MovingAverage there were some results not wrapped into promises leading into the following issues:

Screenshot 2021-03-09 at 21 55 35

Screenshot 2021-03-09 at 21 56 08

Datasource: OpenTSDB

@Gauravshah
Copy link
Member

@ShilpaSivanesan had we not fixed this as part of previous PR ?

@ShilpaSivanesan
Copy link
Contributor

@ShilpaSivanesan had we not fixed this as part of previous PR ?

Yes I added this at this.query
here is the PR #118

@drsoares added this specifically for timeshift.
But our fix should handle this

@drsoares
Copy link
Contributor Author

drsoares commented Mar 10, 2021

Those two parts were missing and not working for OpenTSDB at least. Inside timeshift and movingaverage methods, please double check. I'm assuming that 118 is already released on 0.0.5 version.

@drsoares
Copy link
Contributor Author

Some other datasources are experiencing the same problem.

#124 (comment)

@xpatos
Copy link

xpatos commented Mar 13, 2021

I can confirm that this fix works in my case for elastic search. I just applied the fix in Chrome Dev tools. Please merge this PR.

@Gauravshah
Copy link
Member

@drsoares can you help me understand why does this fix things ?
we are converting all before sending it to grafana

if(ds_res.then){

@earth08
Copy link

earth08 commented Mar 25, 2021

Not working on grafana 7.4.5 with influxdb

@xpatos
Copy link

xpatos commented Mar 31, 2021

@drsoares Can you please help @Gauravshah understand what is the problem so our graphs can start working again.

@earth08
Copy link

earth08 commented Mar 31, 2021

I started using autohome-comparequeries after waiting so long

@drsoares
Copy link
Contributor Author

drsoares commented Apr 7, 2021

Apologies for the long absence. @earth08 do you mean this change or the plugin before the change?

@Gauravshah I think the best explanation is trying to reproduce the error with the mentioned datasources. (influx, opentsdb and elasticsearch) I wonder if there's any in which this plugin is working, not sure if there are any behavioural diferences bettween the query datasources from different plugins, the contract is the same and never returns a promise, it results an observable. I've just let that if statement to make it consistent with the remaining plugin.
Cheers

@earth08
Copy link

earth08 commented Apr 19, 2021

@drsoares
I have never got it working,
In my case arithmetic is working, but timeshif doesnt

@drsoares
Copy link
Contributor Author

@earth08 but have you tried with this patch? Without these changes only arithmetic was working for me as well.
Cheers

@earth08
Copy link

earth08 commented Apr 26, 2021

@drsoares which patch,
As I only see 0.0.5 version
And time shift is not working

@giom-l
Copy link

giom-l commented Apr 26, 2021

I confirm this fix perfectly works with grafana 7.5.2 and metaqueries 0.0.5 (tested with timeshift)

@xpatos
Copy link

xpatos commented Apr 26, 2021

Since @Gauravshah seems reluctant to merge this fix, can we create a fork or use a branch or something. Many graphs in our company is broken due to this bug. It has soon been two months since this bug was reported.

@garyhodgson
Copy link

Can also confirm that manually applying the change in this PR and restarting grafana got timeshift to work again (grafana 7.5.5)

@earth08
Copy link

earth08 commented May 13, 2021

How to apply and what change are u talking about?

@garyhodgson
Copy link

How to apply and what change are u talking about?

I just applied the changes detailed here: https://github.com/GoshPosh/grafana-meta-queries/pull/128/files

  • On the grafana server go to the plugins folder.
  • In the grafana-meta-queries folder is a file called datasource.js. Open this file in an editor.
  • Go to line 202 where it says "return ds.query(options)" and replace it with text:
var ds_res = ds.query(options);
if(ds_res.then){
    return ds_res;
}
else{
    return ds_res.toPromise();
}
  • do the same for line 271.
  • restart grafana.

@elluishinojos
Copy link

Any updates on this? This plugin is really useful, but there is no way to manually update a plugin at Grafana Cloud and this is supposed to be the only plugin available that is supposed to allow to do time shifts

@Gauravshah
Copy link
Member

@drsoares can you remove some of recent commits so that this PR can be merged. we have another competing PR here #132 was trying to avoid merge that

@ShilpaSivanesan
Copy link
Contributor

@drsoares could you please pull the latest master and update this PR
Also please push dist/datasource.js

@ShilpaSivanesan
Copy link
Contributor

ShilpaSivanesan commented Mar 3, 2022

Fixed here #141

@giom-l
Copy link

giom-l commented Mar 3, 2022

I still don't understand why this issue is closed.
I installed tag 0.0.8 some minutes ago, and I still hit the same issue with timeshift function.

runRequest.catchError TypeError: data is undefined
    promise https://grafana/public/plugins/goshposh-metaqueries-datasource/datasource.js:322

When I apply the patch stated above in datasource.js
replace

 return ds.query(options);

with

var ds_res = ds.query(options);
if (ds_res.then){
    return ds_res;
}
else {
    return ds_res.toPromise();
}

on lines 301 & 232
everything is working really fine.

Unless I'm missing something, #141 does not fix that point.

@giom-l
Copy link

giom-l commented Mar 3, 2022

I added a new PR to integrate changes proposed by @drsoares based on uptodate master branch.
I also added some stuffs in docker-compose for testing
#145

@ShilpaSivanesan ShilpaSivanesan merged commit ecdf31e into GoshPosh:master Mar 8, 2022
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.

8 participants