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

POI's not displaying #519

Open
HilaryN opened this issue Feb 18, 2023 · 30 comments
Open

POI's not displaying #519

HilaryN opened this issue Feb 18, 2023 · 30 comments

Comments

@HilaryN
Copy link
Contributor

HilaryN commented Feb 18, 2023

I seem to be having an issue with POI's not displaying, both on my phone and in the emulator (master branch).
@mvl22 @si-the-pie are you aware of any issue server-side?
I've also tried a circular route with POI's but the POI's don't display.

@mvl22
Copy link
Member

mvl22 commented Feb 19, 2023

Can you confirm which POI set you are trying to display, and what part of the country?

Also, can you help me identify the API endpoint being used?

We've made some changes recently but I'm not aware we've changed anything.

@si-the-pie
Copy link
Member

si-the-pie commented Feb 19, 2023

Hilary,

Thanks for the bug report which I have been looking at.

I've checked this on a Fairphone 3 which has CycleStreets version 3.10.0/1639 installed and I have reproduced the problem you have reported.

The CycleStreets API is producing valid geojson for the calls, e.g: these railway stations near Leicester:

{
    "type": "FeatureCollection",
    "features": [
        {
            "type": "Feature",
            "properties": {
                "id": "LEI",
                "name": "Leicester",
                "notes": null,
                "osmTags": null,
                "website": "http://www.nationalrail.co.uk/stations/lei/details.html",
                "distance": 722,
                "iconUrl": "https://www.cyclestreets.net/images/pois/iconsets/osm/transport_train_station2.n.16.png"
            },
            "geometry": {
                "type": "Point",
                "coordinates": [
                    -1.124959,
                    52.631001
                ]
            }
        },
        {
            "type": "Feature",
            "properties": {
                "id": "SWS",
                "name": "South Wigston",
                "notes": null,
                "osmTags": null,
                "website": "http://www.nationalrail.co.uk/stations/sws/details.html",
                "distance": 4812,
                "iconUrl": "https://www.cyclestreets.net/images/pois/iconsets/osm/transport_train_station2.n.16.png"
            },
            "geometry": {
                "type": "Point",
                "coordinates": [
                    -1.13402,
                    52.5824
                ]
            }
        }
    ]
}

But those railway stations are not appearing in the APP.

Is this a problem you have only just noticed, or has it been occuring for some time?

Simon

@HilaryN
Copy link
Contributor Author

HilaryN commented Feb 19, 2023

I'm pretty sure it only started yesterday (that's when I first noticed it) or only in the last couple of days.
I'm sure it was OK on Wednesday.
I mainly looked at cycle parking, cafes. Nothing in the whole of the UK. There's always an issue when you first display, but once you move the map or zoom out then they normally display.
Once I get a mo I'll try and debug the API from the app side to check what's been returned, but with nothing having changed on the app it doesn't seem likely it's at that end.

@oliverlockwood
Copy link
Member

FYI there is an API integration test, which doesn't run in CI, but can manually be run locally (IIRC you need to set a property or env var with an API key to do this).
If the API has changed its responses recently, this could be useful to you when investigating.

@mvl22
Copy link
Member

mvl22 commented Feb 19, 2023

Can you confirm what the API call being used is?

@mvl22
Copy link
Member

mvl22 commented Feb 19, 2023

We've identified a possible issue which is that integer/float data is coming out as native types rather than strings.

https://www.cyclestreets.net/api/v2/pois.locations/
shows the IDs as strings but the actual API result shows this now coming out as native numeric.

Is this likely to be a crash issue on Android or does Android likely just cope with this?

We will probably have to force the old behaviour for now.

@mvl22
Copy link
Member

mvl22 commented Feb 20, 2023

We've reverted to the old string behaviour.

Does this make any difference?

@HilaryN
Copy link
Contributor Author

HilaryN commented Feb 20, 2023

Yes! It's working now. Thank you.

btw I wasn't thinking straight when I said "nothing's changed on the app" - some library could easily have changed. Apologies! But as it happens, you've fixed the problem - thank you :-)

I wonder whether it affected the iPhone app.

@HilaryN
Copy link
Contributor Author

HilaryN commented Feb 20, 2023

Do you need me to do anything at the app end to accommodate the new behaviour so you can put it back to how it was?

@si-the-pie
Copy link
Member

si-the-pie commented Feb 20, 2023

One underlying cause of the issue has been fixed. Over the weekend the main server was upgraded, changing to a newer version of PHP. This has tighter error handling than previously and has lead to several minor issues like this cropping up. In particular count() now throws a fatal error if given a boolean value, previously it returned zero.

The problem is not caused by the string change, so please do not make any changes on this account, as we may well restore the new behaviour (as quoting numbers screams appalling).

Here's a screen shot from an Android FairPhone 3:

Screenshot_20230220-123012

As you can see it is showning cashpoints and police stations.

Another issue I have yet to fix is that no B&Bs are shown (none are currently being served by the CycleStreets pois.locations API). [Actual B&Bs do appear, but they are far rarer than I expected.]

But note also in the snapshot that the background tiles have disappeared. This is reported as a new issue: #520.

@mvl22
Copy link
Member

mvl22 commented Feb 20, 2023

Do you need me to do anything at the app end to accommodate the new behaviour so you can put it back to how it was?

No, please keep the code in line with the published API docs, which match the output.

In due course we may issue a new API version which revamps types across all calls, but that would be a specific development.

@HilaryN
Copy link
Contributor Author

HilaryN commented Mar 31, 2023

@si-the-pie @mvl22 This seems to be happening again.

@HilaryN HilaryN reopened this Mar 31, 2023
@HilaryN
Copy link
Contributor Author

HilaryN commented Mar 31, 2023

Ah, no, it seems to be a different problem - see #521

@HilaryN
Copy link
Contributor Author

HilaryN commented Mar 31, 2023

All working fine once I rebooted my phone.

@HilaryN HilaryN closed this as completed Mar 31, 2023
@HilaryN
Copy link
Contributor Author

HilaryN commented Mar 23, 2024

POI's aren't displaying again. I've tried it on 2 phones, cycle parking and cafes.

@HilaryN HilaryN reopened this Mar 23, 2024
@mvl22
Copy link
Member

mvl22 commented Mar 23, 2024

We're investigating; seems to be an API performance issue. We've had a couple of reports in the last few days.

@si-the-pie
Copy link
Member

I don't think there is a problem with the POI's code, however the main server is doing an install of new data. This has been running since 2pm and will continue into the middle of the night.

I am seeing data coming out of the POIs API so if the problem is due to the busy server this should have cleared by tomorrow. Not very satisfactory but these updates only happen monthly.

@si-the-pie
Copy link
Member

The new data install is still running (and should complete sometime on Sunday late afternoon).

However an internal setting has been discovered that may have explained the poor performance of the POIs API. The setting controlled the key_buffer_cache in MySQL, it was at 16MB but has now been changed to 12GB. Since doing that there are far fewer queries backing up in the logs that I've seen, so perhaps this has solved the issue?

@HilaryN
Copy link
Contributor Author

HilaryN commented Mar 24, 2024

I've tried just now and it still isn't working for me.

@mvl22
Copy link
Member

mvl22 commented Mar 24, 2024

I've tried just now and it still isn't working for me.

Are you able to try this in the simulator and see what errors you are getting?

Also, could you do a request and then let us know what specific area and type you requested, and the exact time (with seconds), so that I can match this with a log request?

@si-the-pie
Copy link
Member

This is a bit of a technical note that I'm chipping in here to give some context.

The fundamental problem lies in the MySQL query that generates this view:

select max(id) as id, 
       floor((st_x(lonLat) - -29.92) / 0.68) as ew, floor((st_y(lonLat) - 36.75) / 0.42) as sn, count(id) as clusterCount
  from csExternal.lib_place
 where
       k = 'tourism'
   AND v = 'artwork'
   AND mbrwithin(lonLat, st_linestringfromtext('linestring(-29.92 36.75, 29.24 67.83)', 0))
 group by ew, sn
 order by id desc, sqrt(pow(43 - ew,2) + pow(37 - sn,2)) asc, clusterCount desc
 limit 400;

Which produces this result:

id ew sn clusterCount
publicart-w999942256 41 40 248
publicart-w999728206 45 16 51
...
publicart-w805505736 43 17 72

400 rows in set (1.34 sec)

Applying explain to the query produces:

id select_type table partitions type possible_keys key key_len ref rows filtered Extra
1 SIMPLE lib_place NULL ref lonLat,index(k,v,name) index(k,v,name) 2 const,const 140455 64.74 Using where; Using temporary; Using filesort

The data is in the lib_place table which has 11 million rows and use a spatial index called lonLat.
It also has an index on fields index(k, v, name), which are the OSM k and v fields of the node or way POI, and the value of any naming tag.
MySQL (and probably most other database systems) cannot create composite indices involving a spatial index.
So MySQL has to choose either the spatial index lonLat, or the multi-column index to initially filter the search.
In the example shown the key column shows that it uses index(k,v,name), and the rows column suggests this leaves 140455 rows to be scanned.

Depending on the size of the bounding box some instances of the query may use the spatial index first. But either way a lot of rows still have to be scanned to complete the query.

Usually the inital calls will prompt the table to be loaded into memory so the POI calls will appear to warm up after that stage, but will go cold later as other resources use the memory.

@HilaryN
Copy link
Contributor Author

HilaryN commented Mar 25, 2024

I've tried just now and it still isn't working for me.

Are you able to try this in the simulator and see what errors you are getting?

Also, could you do a request and then let us know what specific area and type you requested, and the exact time (with seconds), so that I can match this with a log request?

Will do. It might be Friday before I can manage it.

@HilaryN
Copy link
Contributor Author

HilaryN commented Mar 30, 2024

2024-03-30 19:34:18.389 2230-2255 EGL_emulation net.cyclestreets D app_time_stats: avg=734.34ms min=4.47ms max=19859.59ms count=28
2024-03-30 19:34:18.487 2230-2263 api.client...nterceptor net.cyclestreets D Sending request: https://api.cyclestreets.net/v2/pois.locations?fields=id,latitude,longitude,name,notes,osmTags,website&limit=150&type=bikeshops&longitude=0.0&latitude=51.477854882560315&radius=19&key=redacted with headers:
2024-03-30 19:34:18.949 2230-2263 api.client...nterceptor net.cyclestreets D Received HTTP/1.1 200 OK response for: https://api.cyclestreets.net/v2/pois.locations?fields=id,latitude,longitude,name,notes,osmTags,website&limit=150&type=bikeshops&longitude=0.0&latitude=51.477854882560315&radius=19&key=redacted
2024-03-30 19:34:19.032 2230-2263 api.client...nterceptor net.cyclestreets D Sending request: https://api.cyclestreets.net/v2/pois.locations?fields=id,latitude,longitude,name,notes,osmTags,website&limit=150&type=cafes&longitude=0.0&latitude=51.477854882560315&radius=19&key=redacted with headers:
2024-03-30 19:34:19.336 2230-2263 api.client...nterceptor net.cyclestreets D Received HTTP/1.1 200 OK response for: https://api.cyclestreets.net/v2/pois.locations?fields=id,latitude,longitude,name,notes,osmTags,website&limit=150&type=cafes&longitude=0.0&latitude=51.477854882560315&radius=19&key=redacted
2024-03-30 19:34:19.405 2230-2255 EGL_emulation net.cyclestreets D app_time_stats: avg=201.86ms min=8.86ms max=835.78ms count=5

@mvl22
Copy link
Member

mvl22 commented Mar 30, 2024

Thanks, Hilary.

Is there any stack trace error in the emulator?

So we have actually changed the ID format a month ago, which I thought might be most likely, though in theory this should not be a problem because the API documentation did state that the ID is not stable.

Screenshot 2024-03-30 at 22 46 10

I've tested things and just now hotfixed the API code, so that for the Android app key the response matches the original. As you can see, the difference is that the ID has letters also now. Also I've tried setting a NULL notes field to an empty string:

Screenshot 2024-03-30 at 22 46 58

This seems to match the format shown in the tests:

https://github.com/cyclestreets/android/blob/master/libraries/cyclestreets-core/src/test/resources/__files/pois.json

Screenshot 2024-03-30 at 22 47 34

The parser in the Android app code seems to be at:
https://github.com/cyclestreets/android/blob/master/libraries/cyclestreets-core/src/main/java/net/cyclestreets/api/client/geojson/PoiFactory.java#L17

However, even after force-quitting I'm still not seeing POIs appear on my Android.

Can you access errors in the emulator?

Just to say that API requests are responded to pretty immediately after the request, according to the logs - i.e. I am moving the map and see the response in the logs immediately. So I don't think this is a performance issue as Simon was implying could be the issue.

UPDATE: Hmm, twice I did briefly see POIs appear. But I can't reproduce this.

@mvl22
Copy link
Member

mvl22 commented Apr 1, 2024

Feedback:

After weeks of no POI's I whooped with delight when they made a reappearance this afternoon at around five past three. However my celebrations were shortlived as they went again after a few minutes :( Please please fix this issue

Seems my change at the API end partially addressed the issue but there some other issue remaining.

@HilaryN
Copy link
Contributor Author

HilaryN commented Apr 4, 2024

I'm struggling with a slow laptop and lack of time :-) but I've got as far as being able to see that data is definitely being returned and some issue is occurring when processing it. I'll persevere...
Edit: ah yes, it does seem to be expecting an integer for the ID. OK, I'll try and sort it. I think Sunday will be my next window.

@mvl22
Copy link
Member

mvl22 commented Apr 4, 2024

it does seem to be expecting an integer for the ID

Thanks. I know the feeling :) Is that an integer expressed as a string?

If you can let me know exactly what is expected I can patch the at the API end for now.

@HilaryN
Copy link
Contributor Author

HilaryN commented Apr 8, 2024

Apologies, for some reason I missed this message.
I think this is where it's failing, in PoiFactory:
new POI(Integer.parseInt(feature.getProperty("id")), etc
I think it uses the integer to create a hashcode.
Actually, I think it's just that the number is too big for an integer, e.g. id= "10259345989"

@HilaryN
Copy link
Contributor Author

HilaryN commented Apr 27, 2024

Created new issue: #554

@mvl22
Copy link
Member

mvl22 commented Apr 27, 2024

Actually, I think it's just that the number is too big for an integer, e.g. id= "10259345989"

Ah right, that would explain why this was only working sometimes.

I have taken out the hotfix, so you are getting the pure API out now, i.e. IDs do look like:

"cycleparking-n943984"

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

No branches or pull requests

4 participants