-
Notifications
You must be signed in to change notification settings - Fork 34
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
Error loading aggregate data, averages not available #288
Comments
@jf87 great question! The answer is multi-part and I know the answer for some parts and not for others :)
https://github.com/e-mission/e-mission-phone/blob/master/www/js/metrics.js#L29
Probably good to look at happy to take a quick look since I am pretty familiar with the e-mission code and can there' s a good chance that I can quickly estimate the error location. |
@jf87 Are you still seeing this error? Or does it disappear once you get more data? I haven't seen it once we have sufficient data on the server, so maybe it went away?... |
Sorry for the delayed reply. I am working tomorrow again on this and I'll let you know. (but i think it's still there) |
This is the issue I think.
|
so I just created a new server (actually multiple new servers) while testing e-mission/e-mission-server#637 and e-mission/e-mission-docker#3) and I can't reproduce this even with only one user. I looked at the code, and the popup only occurs when the server returns an error.
|
I also connected a debugger to the client and inserted a breakpoint in at the first line in the success block ( comparing this to the error reported in #288 (comment) it looks like the main difference is that:
|
can you look at the server logs and see if there is anything there? |
I also compared the status of the response. In the working case
in the error popup above
|
since the call is through the angular
we can look at the according to the comments there,
So could it just be that the call timed out while connecting to the server? Can you check the server logs to see whether it received a request at around the time of the error, and if so, what happened to it? |
The error popup #288 (comment) does not have an |
Server shows this: |
Ok. I found the reason:
It is necessary to adapt www/index.html of the cordova app in case of a separate server instance.
I guess It might be good to collect these places somewhere in the docs. |
aha! that's why I have only seen this periodically - most of the other groups that used their own servers turned off the aggregate code. TripAware might have been the only one that kept it.
It is in the docs, although maybe not in a very organized place If you can add this information to the doc (and potentially rename it for clarity), it would be great! |
Ionic is actually javascript framework on top of angular. We used to use them for push notifications, but now we use firebase, and the mapping happens all on the server side. So we should be able to remove |
Also, I have a longer-term question. Background: The reason that the aggregate call is affected by this while the user call is not is because And the reason that we used But now, we know that is not a good model for anything other than heatmaps, and even for heatmaps, you need to be careful. The new vision is to have an aggregator, and queries that authenticate themselves with the aggregator. Question: (@jf87 @PatGendre) So should we start going towards this model by requiring authentication for the aggregate data as well? The effect would be that the aggregate data would only be visible to the community of contributors. So if I submit data, I can see aggregate data, otherwise I can't. This would not be a hard change, and it would also allow us to use the native plugin for all server calls, which would resolve this issue. Thoughts? |
@deepalics0044 I think you ran into this when you set up your own server initially? |
Yes. I think the better solution would be to use the native plugin. |
Will probably only implement the first two options (aggregate data is available publicly v/s only to participants) initially to avoid over-engineering. Will try to get to this after merging the GIS based mode detection. |
Yes sure. I just wanted to hint that this option is flexible enough to implement various schemes :-) |
I agree with Jonathan "Access control should happen on the server side and the individual e-mission deployer should be able to decide [how] aggregate data is available." |
I'm making some changes to the web interface, so decided to tackle some parts of this at the same time. Quick response to @PatGendre wrt
This is already supported. Check out the "Analysis" or "Metrics" tabs on the existing web UI. And because the phone app uses HTML + javascript, we are able to largely re-use the same code on the phone and web apps. IIRC, the main differences are in the HTML, mainly because I am not very good at responsive UX. In fact, the initial and current version (before my pending changes) was that user specific information would only be available through the phone app, and the aggregate information would be publicly available through both the phone and web apps. |
@shankari : thanks! I'll be watching the improvements you are bringing to the web interface. The discussion above with Jonathan gets clearer to me now. |
@shankari |
As discussed yesterday our problem is not related to the need to adapt the www/index.html of the cordova app in case of a separate server instance. (in <meta http-equiv="Content-Security-Policy" ). |
Sure. I left it open for the long-term issue, but I guess we can file a new issue for that. |
The switch to WKWebView forced the use of CORS. There is no workaround. https://ionicframework.com/docs/v3/wkwebview/#cors We didn't encounter this in covid-19 repo since we made all calls through the built-in native plugin that automatically adds the user token and sends out the call. In emission, though, we need make aggregate calls that should not have a user token. We had been using XHR for this (through the angular `$http` server) and all those calls broke. While we could make the appropriate change on the server side, that would not be consistent with our long term goal (e-mission/e-mission-docs#408, e-mission/e-mission-docs#288) So for now, I use the alternative documented here https://ionicframework.com/docs/v3/wkwebview/#i-cant-implement-cors Concretely: - add the native plugin - add a new method `CommHelper.getAggregateData` that wraps it in a promise - change all instances of `$http.post` -> `CommHelper.getAggregateData` - use the configured connectUrl instead of hardcoding - remove the hardcoded URL from index.html Bonus fix: - Dealt with error in the heatmap if there was no data and thus, no bounds by checking to see if the bounds were valid Testing done: Navigated through the app screens until all the aggregate calls were invoked (see below). No errors in the console. ``` [Log] getting aggregate data without user authentication from http://localhost:8080/result/metrics/timestamp with arguments {"freq":"D","start_time":1586131200,"end_time":1587254400,"metric_list":["duration","median_speed","count","distance"],"is_return_aggregate":true} (cordova.js, line 1540) [Log] getting aggregate data without user authentication from http://localhost:8080/result/heatmap/incidents/timestamp with arguments {"start_time":1587187062,"end_time":1587359861,"sel_region":null} (cordova.js, line 1540) [Log] getting aggregate data without user authentication from http://localhost:8080/result/heatmap/pop.route/local_date with arguments {"modes":null,"from_local_date":{"year":2020,"month":4,"day":17,"hour":22},"to_local_date":{"year":2020,"month":4,"day":18,"hour":22},"sel_region":null} (cordova.js, line 1540) [Log] getting aggregate data without user authentication from http://localhost:8080/result/heatmap/incidents/local_date with arguments {"modes":null,"from_local_date":{"year":2020,"month":4,"day":17,"hour":22},"to_local_date":{"year":2020,"month":4,"day":18,"hour":22},"sel_region":null} (cordova.js, line 1540) ```
I get that error every time when the app is open for the first time and after it has been killed.
![img_6768](https://user-images.githubusercontent.com/2108882/51030664-6d098c80-159a-11e9-83ae-ac3b390b6382.PNG)
I replaced the hard-coded e-mission url in metrics.js with my own server, but it does not seem to help.
What data is this supposed to get?
Thanks :-)
The text was updated successfully, but these errors were encountered: