From ef04e156fd0d5905327d62c12e95f66e539f7d5b Mon Sep 17 00:00:00 2001 From: William Woods Date: Mon, 3 Apr 2023 04:26:18 +0000 Subject: [PATCH 1/3] Refactored code for performance and readability --- .../clients/geoduck/UserLocationFetcher.scala | 61 +++++++------------ 1 file changed, 21 insertions(+), 40 deletions(-) diff --git a/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/UserLocationFetcher.scala b/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/UserLocationFetcher.scala index 706ae5143..d83208033 100644 --- a/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/UserLocationFetcher.scala +++ b/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/UserLocationFetcher.scala @@ -1,58 +1,39 @@ -package com.twitter.follow_recommendations.common.clients.geoduck - -import com.twitter.finagle.stats.StatsReceiver import com.twitter.follow_recommendations.common.models.GeohashAndCountryCode import com.twitter.stitch.Stitch -import javax.inject.Inject -import javax.inject.Singleton +import javax.inject.{Inject, Singleton} @Singleton class UserLocationFetcher @Inject() ( locationServiceClient: LocationServiceClient, reverseGeocodeClient: ReverseGeocodeClient, statsReceiver: StatsReceiver) { + private val stats = statsReceiver.scope("user_location_fetcher") - private val stats: StatsReceiver = statsReceiver.scope("user_location_fetcher") - private val totalRequestsCounter = stats.counter("requests") - private val emptyResponsesCounter = stats.counter("empty") - private val locationServiceExceptionCounter = stats.counter("location_service_exception") - private val reverseGeocodeExceptionCounter = stats.counter("reverse_geocode_exception") + def getGeohashAndCountryCode(userId: Option[Long], ipAddress: Option[String]): Stitch[Option[GeohashAndCountryCode]] = { + val totalRequestsCounter = stats.counter("requests").incr() - def getGeohashAndCountryCode( - userId: Option[Long], - ipAddress: Option[String] - ): Stitch[Option[GeohashAndCountryCode]] = { - totalRequestsCounter.incr() - val lscLocationStitch = Stitch - .collect { - userId.map(locationServiceClient.getGeohashAndCountryCode) - }.rescue { - case _: Exception => - locationServiceExceptionCounter.incr() - Stitch.None - } + val lscLocationStitch = Stitch.collect(userId.map(locationServiceClient.getGeohashAndCountryCode)).rescue { + case _: Exception => + stats.counter("location_service_exception").incr() + Stitch.None + } - val ipLocationStitch = Stitch - .collect { - ipAddress.map(reverseGeocodeClient.getGeohashAndCountryCode) - }.rescue { - case _: Exception => - reverseGeocodeExceptionCounter.incr() - Stitch.None - } + val ipLocationStitch = Stitch.collect(ipAddress.map(reverseGeocodeClient.getGeohashAndCountryCode)).rescue { + case _: Exception => + stats.counter("reverse_geocode_exception").incr() + Stitch.None + } Stitch.join(lscLocationStitch, ipLocationStitch).map { - case (lscLocation, ipLocation) => { - val geohash = lscLocation.flatMap(_.geohash).orElse(ipLocation.flatMap(_.geohash)) - val countryCode = - lscLocation.flatMap(_.countryCode).orElse(ipLocation.flatMap(_.countryCode)) - (geohash, countryCode) match { - case (None, None) => - emptyResponsesCounter.incr() + case (lscLocation, ipLocation) => + (lscLocation.flatMap(_.geohash).orElse(ipLocation.flatMap(_.geohash)), + lscLocation.flatMap(_.countryCode).orElse(ipLocation.flatMap(_.countryCode))) match { + case (Some(geohash), Some(countryCode)) => + Some(GeohashAndCountryCode(geohash, countryCode)) + case _ => + stats.counter("empty").incr() None - case _ => Some(GeohashAndCountryCode(geohash, countryCode)) - } } } } From d73c0bdf732e88657775a853bb73fd69a322520c Mon Sep 17 00:00:00 2001 From: William Woods Date: Mon, 3 Apr 2023 04:31:24 +0000 Subject: [PATCH 2/3] Set to main --- .../clients/geoduck/UserLocationFetcher.scala | 61 ++++++++++++------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/UserLocationFetcher.scala b/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/UserLocationFetcher.scala index d83208033..706ae5143 100644 --- a/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/UserLocationFetcher.scala +++ b/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/UserLocationFetcher.scala @@ -1,39 +1,58 @@ +package com.twitter.follow_recommendations.common.clients.geoduck + +import com.twitter.finagle.stats.StatsReceiver import com.twitter.follow_recommendations.common.models.GeohashAndCountryCode import com.twitter.stitch.Stitch -import javax.inject.{Inject, Singleton} +import javax.inject.Inject +import javax.inject.Singleton @Singleton class UserLocationFetcher @Inject() ( locationServiceClient: LocationServiceClient, reverseGeocodeClient: ReverseGeocodeClient, statsReceiver: StatsReceiver) { - private val stats = statsReceiver.scope("user_location_fetcher") - def getGeohashAndCountryCode(userId: Option[Long], ipAddress: Option[String]): Stitch[Option[GeohashAndCountryCode]] = { - val totalRequestsCounter = stats.counter("requests").incr() + private val stats: StatsReceiver = statsReceiver.scope("user_location_fetcher") + private val totalRequestsCounter = stats.counter("requests") + private val emptyResponsesCounter = stats.counter("empty") + private val locationServiceExceptionCounter = stats.counter("location_service_exception") + private val reverseGeocodeExceptionCounter = stats.counter("reverse_geocode_exception") - val lscLocationStitch = Stitch.collect(userId.map(locationServiceClient.getGeohashAndCountryCode)).rescue { - case _: Exception => - stats.counter("location_service_exception").incr() - Stitch.None - } + def getGeohashAndCountryCode( + userId: Option[Long], + ipAddress: Option[String] + ): Stitch[Option[GeohashAndCountryCode]] = { + totalRequestsCounter.incr() + val lscLocationStitch = Stitch + .collect { + userId.map(locationServiceClient.getGeohashAndCountryCode) + }.rescue { + case _: Exception => + locationServiceExceptionCounter.incr() + Stitch.None + } - val ipLocationStitch = Stitch.collect(ipAddress.map(reverseGeocodeClient.getGeohashAndCountryCode)).rescue { - case _: Exception => - stats.counter("reverse_geocode_exception").incr() - Stitch.None - } + val ipLocationStitch = Stitch + .collect { + ipAddress.map(reverseGeocodeClient.getGeohashAndCountryCode) + }.rescue { + case _: Exception => + reverseGeocodeExceptionCounter.incr() + Stitch.None + } Stitch.join(lscLocationStitch, ipLocationStitch).map { - case (lscLocation, ipLocation) => - (lscLocation.flatMap(_.geohash).orElse(ipLocation.flatMap(_.geohash)), - lscLocation.flatMap(_.countryCode).orElse(ipLocation.flatMap(_.countryCode))) match { - case (Some(geohash), Some(countryCode)) => - Some(GeohashAndCountryCode(geohash, countryCode)) - case _ => - stats.counter("empty").incr() + case (lscLocation, ipLocation) => { + val geohash = lscLocation.flatMap(_.geohash).orElse(ipLocation.flatMap(_.geohash)) + val countryCode = + lscLocation.flatMap(_.countryCode).orElse(ipLocation.flatMap(_.countryCode)) + (geohash, countryCode) match { + case (None, None) => + emptyResponsesCounter.incr() None + case _ => Some(GeohashAndCountryCode(geohash, countryCode)) + } } } } From 37ec1935eadb3000c113c7348760f4d3dedc3256 Mon Sep 17 00:00:00 2001 From: William Woods Date: Mon, 3 Apr 2023 07:22:50 +0000 Subject: [PATCH 3/3] Refactored code that improves performance and readability --- .../geoduck/ReverseGeocodeClient.scala | 50 +++++-------------- 1 file changed, 12 insertions(+), 38 deletions(-) diff --git a/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/ReverseGeocodeClient.scala b/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/ReverseGeocodeClient.scala index 576359128..628ad9a7f 100644 --- a/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/ReverseGeocodeClient.scala +++ b/follow-recommendations-service/common/src/main/scala/com/twitter/follow_recommendations/common/clients/geoduck/ReverseGeocodeClient.scala @@ -1,7 +1,6 @@ package com.twitter.follow_recommendations.common.clients.geoduck import com.twitter.follow_recommendations.common.models.GeohashAndCountryCode -import com.twitter.geoduck.common.thriftscala.Location import com.twitter.geoduck.common.thriftscala.PlaceQuery import com.twitter.geoduck.common.thriftscala.ReverseGeocodeIPRequest import com.twitter.geoduck.service.thriftscala.GeoContext @@ -12,46 +11,21 @@ import javax.inject.Singleton @Singleton class ReverseGeocodeClient @Inject() (rgcService: ReverseGeocoder.MethodPerEndpoint) { - def getGeohashAndCountryCode(ipAddress: String): Stitch[GeohashAndCountryCode] = { - Stitch - .callFuture { - rgcService - .reverseGeocodeIp( - ReverseGeocodeIPRequest( - Seq(ipAddress), - PlaceQuery(None), - simpleReverseGeocode = true - ) // note: simpleReverseGeocode means that country code will be included in response - ).map { response => - response.found.get(ipAddress) match { - case Some(location) => getGeohashAndCountryCodeFromLocation(location) - case _ => GeohashAndCountryCode(None, None) - } - } - } - } - - private def getGeohashAndCountryCodeFromLocation(location: Location): GeohashAndCountryCode = { - val countryCode: Option[String] = location.simpleRgcResult.flatMap { _.countryCodeAlpha2 } - - val geohashString: Option[String] = location.geohash.flatMap { hash => - hash.stringGeohash.flatMap { hashString => - Some(ReverseGeocodeClient.truncate(hashString)) + def getGeohashAndCountryCode(ipAddress: String): Stitch[GeohashAndCountryCode] = + Stitch.callFuture { + rgcService.reverseGeocodeIp( + ReverseGeocodeIPRequest(Seq(ipAddress), PlaceQuery(None), simpleReverseGeocode = true) + ).map { response => + response.found.get(ipAddress) match { + case Some(location) => getGeohashAndCountryCodeFromLocation(location) + case _ => GeohashAndCountryCode(None, None) + } } } + private def getGeohashAndCountryCodeFromLocation(location: Location): GeohashAndCountryCode = { + val countryCode = location.simpleRgcResult.flatMap(_.countryCodeAlpha2) + val geohashString = location.geohash.flatMap(_.stringGeohash.map(_.take(4))) GeohashAndCountryCode(geohashString, countryCode) } - -} - -object ReverseGeocodeClient { - - val DefaultGeoduckIPRequestContext: GeoContext = - GeoContext(allPlaceTypes = true, includeGeohash = true, includeCountryCode = true) - - // All these geohashes are guessed by IP (Logical Location Source). - // So take the four letters to make sure it is consistent with LocationServiceClient - val GeohashLengthAfterTruncation = 4 - def truncate(geohash: String): String = geohash.take(GeohashLengthAfterTruncation) }