From c5d9db61e64686ad24a6c7d877dd2d4b169748e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Ignacio=20S=C3=A1nchez=20Lara?= Date: Mon, 23 Jul 2018 15:48:32 +0200 Subject: [PATCH] Mapbox error handling --- .../cartodb_services/geocoder.py | 18 +++++++-- .../cartodb_services/mapbox/bulk_geocoder.py | 4 +- .../cartodb_services/mapbox/geocoder.py | 37 ++++++++++++------- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/geocoder.py index f23695f..cde5182 100644 --- a/server/lib/python/cartodb_services/cartodb_services/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/geocoder.py @@ -9,9 +9,6 @@ import json PRECISION_PRECISE = 'precise' PRECISION_INTERPOLATED = 'interpolated' -EMPTY_RESPONSE = [[], {}] - - def geocoder_metadata(relevance, precision, match_types): return { 'relevance': round(relevance, 2), @@ -20,6 +17,18 @@ def geocoder_metadata(relevance, precision, match_types): } +def geocoder_error_response(message): + return [[], {'error': message}] + + +# Single empty result +EMPTY_RESPONSE = [[], {}] +# HTTP 429 and related +TOO_MANY_REQUESTS_ERROR_RESPONSE = geocoder_error_response('Rate limit exceeded') +# Full empty _batch_geocode response +EMPTY_BATCH_RESPONSE = [] + + def compose_address(street, city=None, state=None, country=None): return ', '.join(filter(None, [street, city, state, country])) @@ -42,7 +51,7 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org service_manager.assert_within_limits(quota=False) geocode_results = geocoder.bulk_geocode(searches) results = [] - if geocode_results: + if not geocode_results == EMPTY_BATCH_RESPONSE: for result in geocode_results: if len(result) > 2: metadata = result[2] @@ -65,6 +74,7 @@ def run_street_point_geocoder(plpy, GD, geocoder, service_manager, username, org results.append([result[0], None, json.dumps(metadata)]) empty_count = len(searches) - success_count - failed_count + logger.debug("--> Success: {}; empty: {}; failed: {}".format(success_count, empty_count, failed_count)) service_manager.quota_service.increment_success_service_use(success_count) service_manager.quota_service.increment_empty_service_use(empty_count) service_manager.quota_service.increment_failed_service_use(failed_count) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py index 2563d43..a939716 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/bulk_geocoder.py @@ -1,8 +1,6 @@ -import json, requests, time -from requests.adapters import HTTPAdapter +import requests from cartodb_services import StreetPointBulkGeocoder from cartodb_services.mapbox import MapboxGeocoder -from cartodb_services.tools.exceptions import ServiceException from iso3166 import countries from cartodb_services.tools.country import country_to_iso3 diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 2af2c6f..39d9da5 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -5,7 +5,7 @@ Python client for the Mapbox Geocoder service. import json import requests from mapbox import Geocoder -from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE +from cartodb_services.geocoder import PRECISION_PRECISE, PRECISION_INTERPOLATED, geocoder_metadata, EMPTY_RESPONSE, EMPTY_BATCH_RESPONSE, TOO_MANY_REQUESTS_ERROR_RESPONSE, geocoder_error_response from cartodb_services.metrics import Traceable from cartodb_services.tools.exceptions import ServiceException from cartodb_services.tools.qps import qps_retry @@ -69,7 +69,7 @@ class MapboxGeocoder(Traceable): result.append(EMPTY_RESPONSE) return result else: - return EMPTY_RESPONSE + return EMPTY_BATCH_RESPONSE def _extract_lng_lat_from_feature(self, feature): geometry = feature[ENTRY_GEOMETRY] @@ -118,9 +118,17 @@ class MapboxGeocoder(Traceable): :param city: :param state_province: :param country: Country ISO 3166 code - :return: [x, y] on success, [] on error + :return: [x, y] on success, raises ServiceException on error """ - return self.geocode_meta(searchtext, city, state_province, country)[0] + response = self.geocode_meta(searchtext, city, state_province, country) + if response: + error_message = response[1].get('error', None) + if error_message: + raise ServiceException(error_message, None) + else: + return response[0] + else: + return EMPTY_RESPONSE @qps_retry(qps=10) def geocode_meta(self, searchtext, city=None, state_province=None, @@ -138,7 +146,8 @@ class MapboxGeocoder(Traceable): free_search = ', '.join(address) - return self.geocode_free_text_meta([free_search], country)[0] + response = self.geocode_free_text_meta([free_search], country) + return response[0] if response else EMPTY_RESPONSE @qps_retry(qps=10) def geocode_free_text_meta(self, free_searches, country=None): @@ -157,24 +166,26 @@ class MapboxGeocoder(Traceable): if response.status_code == requests.codes.ok: return self._parse_geocoder_response(response.text) + elif response.status_code == requests.codes.too_many_requests: + return [TOO_MANY_REQUESTS_ERROR_RESPONSE] * len(free_searches) elif response.status_code == requests.codes.bad_request: - return EMPTY_RESPONSE + return EMPTY_BATCH_RESPONSE elif response.status_code == requests.codes.unprocessable_entity: - return EMPTY_RESPONSE + return EMPTY_BATCH_RESPONSE else: - raise ServiceException(response.status_code, response) + msg = "Unkown status: {}".format(response.status_code) + return [geocoder_error_response(msg)] * len(free_searches) except requests.Timeout as te: # In case of timeout we want to stop the job because the server # could be down - self._logger.error('Timeout connecting to Mapbox geocoding server', - te) - raise ServiceException('Error geocoding {0} using Mapbox'.format( - free_search), None) + msg = 'Timeout connecting to Mapbox geocoding server' + self._logger.error(msg, te) + return [geocoder_error_response(msg)] * len(free_searches) except requests.ConnectionError as ce: # Don't raise the exception to continue with the geocoding job self._logger.error('Error connecting to Mapbox geocoding server', exception=ce) - return EMPTY_RESPONSE + return EMPTY_BATCH_RESPONSE def _escape(self, free_search): # Semicolon is used to separate batch geocoding; there's no documented