IAB Robots & Spiders enrichment #3750

rzats · 2018-05-02T13:02:15Z

Closes #937.

Still needs some work:

test this in an actual pipeline
figure out how to determine when basic HTTP authentication is necessary and apply the username/password

but should be ready for an initial review.

BenFradet

Looks good overall, please introduce more tests 👍

...main/scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/EnrichmentRegistry.scala

@@ -168,6 +169,8 @@ object EnrichmentRegistry {
            SqlQueryEnrichmentConfig.parse(enrichmentConfig, schemaKey).map((nm, _).some)
          } else if (nm == "pii_enrichment_config") {
            PiiPseudonymizerEnrichment.parse(enrichmentConfig, schemaKey).map((nm, _).some)
+          } else if (nm == "iab_spiders_and_robots_enrichment") {


...main/scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/EnrichmentRegistry.scala

+   * @return Option boxing the IabEnrichment instance
+   */
+  def getIabEnrichment: Option[IabEnrichment] =
+    getEnrichment[IabEnrichment]("iab_spiders_and_robots_enrichment")


.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+                                        "iab_spiders_and_robots_enrichment",
+                                        "jsonschema",
+                                        1,
+                                        0)


BenFradet · 2018-05-03T19:13:24Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+
+// Scalaz
+import scalaz.Scalaz._
+import scalaz._


the imports are:

import scalaz._ import Scalaz._

BenFradet · 2018-05-03T19:13:46Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+   * @return a configured IabEnrichment instance
+   */
+  def parse(config: JValue, schemaKey: SchemaKey, localMode: Boolean): ValidatedNelMessage[IabEnrichment] =
+    isParseable(config, schemaKey).flatMap(conf => {


unnecessary parens

BenFradet · 2018-05-03T19:17:13Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+
+  private type DbEntry = Option[(Option[URI], String)]
+  private val schemaUri =
+    "iglu:com.snowplowanalytics.snowplow.enrichments/iab_spiders_and_robots_enrichment/jsonschema/1-0-0"


seems to be the wrong coordinates here too

This one should actually be pointing to the response POJO, will fix

BenFradet · 2018-05-03T19:18:58Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+  private val iabClient = {
+    def file(db: DbEntry): File = new File(db.get._2)
+
+    new IabClient(file(dbs._1), file(dbs._2), file(dbs._3))


it's worth introducing a test which references not existing files

BenFradet · 2018-05-03T19:19:52Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+  private[enrichments] def performCheck(userAgent: String,
+                                        ipAddress: String,
+                                        accurateAt: DateTime): Validation[Throwable, IabResponse] =
+    Validation.fromTryCatch(iabClient.checkAt(userAgent, InetAddress.getByName(ipAddress), accurateAt.toDate))


I think this is unsafe with the scalaz version we're using as it will also catch exceptions that are not NonFatal

BenFradet · 2018-05-03T19:20:29Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+    try {
+      getIab(userAgent, ipAddress, accurateAt).map(addSchema)
+    } catch {
+      case NonFatal(exc) => exc.toString.fail


BenFradet · 2018-05-03T19:21:24Z

...la/com.snowplowanalytics.snowplow.enrich.common/enrichments/registry/IabEnrichmentSpec.scala

+                (check.getPrimaryImpact.toString must_== expectedPrimaryImpact)
+          }
+        }
+    }


this is too light testing

Will fix this. Is there a way to do integration tests that interact directly with EnrichmentManager and send in Snowplow events? (instead of method params)

integration tests are usually done in stream enrich or spark enrich

alexanderdean · 2018-05-03T19:23:06Z

Thanks @BenFradet - Rostyslav please action Ben's feedback...

rzats · 2018-05-09T20:47:02Z

@BenFradet @chuwy just finished this round of QA, should be ready for another review

BenFradet

we're getting there 👍 , what I would do is log a separate ticket to handle integration tests in spark enrich for example

BenFradet · 2018-05-10T09:05:29Z

.../main/scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/EnrichmentManager.scala

+                         Option(event.user_ipaddress),
+                         Option(event.derived_tstamp).map(EventEnrichments.fromTimestamp))
+          .map(_.some)
+      }


no need for curlies

BenFradet · 2018-05-10T09:17:05Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+      (uri.toValidationNel |@| db.toValidationNel) { (uri, db) =>
+        for {
+          u <- getDatabaseUri(uri, db).toValidationNel: ValidatedNelMessage[URI]
+        } yield (name, u, db)


(uri.toValidationNel |@| db.toValidationNel) { (uri, db) => getDatabaseUri(uri, db).toValidationNel.map(u => (name, u, db)) }

modulo type inference

BenFradet · 2018-05-10T09:18:00Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+          u <- getDatabaseUri(uri, db).toValidationNel: ValidatedNelMessage[URI]
+        } yield (name, u, db)
+
+      }.flatMap(x => x).some


ah yes, you can use identity instead of x => x

BenFradet · 2018-05-10T09:24:52Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+   */
+  private def getDatabaseUri(uri: String, database: String): ValidatedMessage[URI] =
+    ConversionUtils
+      .stringToUri(uri + "/" + database)


I would shy away from doing path handling using string concatenation, what if uri has a / at the end? does ConversionUtils handle it?

It should work with trailing slashes: https://github.com/snowplow/snowplow/blob/master/3-enrich/scala-common-enrich/src/test/scala/com.snowplowanalytics.snowplow.enrich.common/utils/conversionUtilsSpecs.scala#L73

this example doesn't have two trailing slashes afaict

Seems to work fine (the multiple slashes are simply added to the URI with no errors). I've added a normalize call here just in case - that should remove the extra slashes.

normalize doesn't deal with the case I meant, doing string concatenation on paths can result in:

val uri = "http://google.com/" val databse = "database" uri + "/" + database // http://google.com//database ConversionUtils.stringToUri(uri + "/" + database) // Success(Some(http://google.com//database)) new URI("http://google.com/").resolve("/" + database) // http://google.com/database

Do you see what I mean now?

BenFradet · 2018-05-10T09:43:42Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+    def db(dbPath: FileTuple): DbEntry = dbPath.map {
+      case (name, uri, file) =>
+        if (localMode) {
+          (None, getClass.getResource(file).toURI.getPath)


mmmh even if this is only for tests maybe we should make it safer wrt to npes

BenFradet · 2018-05-10T09:44:29Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+   */
+  private[enrichments] def performCheck(userAgent: String,
+                                        ipAddress: String,
+                                        accurateAt: DateTime): Validation[Throwable, IabEnrichmentResponse] =


should this be occuredAt?

occurred even

These are the parameter names used by the IAB client: https://github.com/snowplow/iab-spiders-and-robots-java-client/blob/master/src/main/java/com/snowplowanalytics/iab/spidersandrobotsclient/IabClient.java#L77

I could of course rename this but that would be a bit inconsistent.

then I think it's a typo in the client since accurateAt being a DateTime doesn't make sense to me?

BenFradet · 2018-05-10T09:45:35Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+                                        ipAddress: String,
+                                        accurateAt: DateTime): Validation[Throwable, IabEnrichmentResponse] =
+    try {
+      val result = iabClient.checkAt(userAgent, InetAddress.getByName(ipAddress), accurateAt.toDate)


what if it's not an ip address? I'd rather we correctly type our failure states instead of blanket wrapping in try/catch

BenFradet · 2018-05-10T09:46:22Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+      IabEnrichmentResponse(result.isSpiderOrRobot,
+                            result.getCategory.toString,
+                            result.getReason.toString,
+                            result.getPrimaryImpact.toString).success


can we keep the types further? i.e. no toString here

The reason this is converted into a scala case class is because those types of Java enums seem to have some issues when they're serialized via Extraction.decompose(). I could convert the IabResponse into an IabEnrichmentResponse in getIab instead but I don't think that would make much of a difference.

BenFradet · 2018-05-10T09:48:52Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+      getIab(userAgent, ipAddress, accurateAt).map(addSchema)
+    } catch {
+      case NonFatal(exc) => s"${exc.getMessage}".fail
+    }


same here, this is our code, we shouldn't need try catch at this level

BenFradet · 2018-05-10T09:49:55Z

3-enrich/scala-common-enrich/src/main/scala/com.snowplowanalytics.snowplow.enrich/package.scala

+   * Type alias for an Option-boxed
+   * Tuple3 of a file location
+   */
+  type FileTuple = Option[(String, URI, String)]


This type alias doesn't provide any benefits as we can't infer what's inside from the name

Should I use a case class then?

BenFradet · 2018-05-10T13:34:02Z

Also since this enrichment relies on local files, they'll need to be cached, e.g. for spark enrich: https://github.com/snowplow/snowplow/blob/master/3-enrich/spark-enrich/src/main/scala/com.snowplowanalytics.snowplow.enrich.spark/EnrichJob.scala#L179-L183

chuwy

Hey @rzats, LGTM! Well done.

chuwy · 2018-05-10T17:52:36Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+                            result.getReason.toString,
+                            result.getPrimaryImpact.toString).success
+    } catch {
+      case NonFatal(exc) => exc.fail


.fail is deprecated, try to stick with .failure (few more places)

chuwy · 2018-05-10T17:54:22Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+              case obj: JObject => obj.success
+              case _            => s"Couldn't transform IAB response $response into JSON".fail
+            }
+          case Failure(throwable) => s"${throwable.getMessage}".fail


throwable.getMessage.failure (quotes and curlies are redundant)

chuwy · 2018-05-10T17:55:38Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+   * @param context IAB context as JSON Object
+   * @return JSON Object wrapped as Self-describing JSON
+   */
+  private def addSchema(context: JObject): JObject =


I'm kind of surprised we don't have this method defined somewhere.

BenFradet · 2018-05-28T16:07:44Z

.../spark-enrich/src/test/scala/com.snowplowanalytics.snowplow.enrich.spark/EnrichJobSpec.scala

+         |}""".format(database, database match {
+        case "ipFile"            => "ip_exclude_current_cidr.txt"
+        case "excludeUseragentFile"            => "exclude_current.txt"
+        case "includeUseragentFile"         => "include_current.txt"


weird indent

BenFradet · 2018-05-28T16:13:20Z

...rc/test/scala/com.snowplowanalytics.snowplow.enrich.spark/good/IabEnrichmentCfLineSpec.scala

+      for (idx <- IabEnrichmentCfLineSpec.expected.indices) {
+        actual(idx) must BeFieldEqualTo(IabEnrichmentCfLineSpec.expected(idx), idx)
+      }
+    }


could we have a test that is a bot?

BenFradet · 2018-05-29T09:57:06Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+   */
+  private def getDatabaseUri(uri: String, database: String): ValidatedMessage[URI] =
+    ConversionUtils
+      .stringToUri(uri + "/" + database)


normalize doesn't deal with the case I meant, doing string concatenation on paths can result in:

val uri = "http://google.com/" val databse = "database" uri + "/" + database // http://google.com//database ConversionUtils.stringToUri(uri + "/" + database) // Success(Some(http://google.com//database)) new URI("http://google.com/").resolve("/" + database) // http://google.com/database

Do you see what I mean now?

BenFradet · 2018-05-29T09:59:59Z

...om.snowplowanalytics.snowplow.enrich.common/enrichments/registry/EnrichmentConfigsSpec.scala

+        "parameters": {
+          "ipFile": {
+            "database": "ip_exclude_current_cidr.txt",
+            "uri": "https://example.com"


let's add a test incorporating my remark above

that was meant to add a / at the end of uri to check uri resolving works properly

BenFradet · 2018-05-29T10:01:36Z

.../spark-enrich/src/test/scala/com.snowplowanalytics.snowplow.enrich.spark/EnrichJobSpec.scala

+              |"embedded": {
+                |"path": "/iglu"
+              |}
+            |}


please remove before release

BenFradet · 2018-05-29T10:03:07Z

...t/scala/com.snowplowanalytics.snowplow.enrich.spark/good/IabEnrichmentSpiderCfLineSpec.scala

+    "not write any bad rows" in {
+      dirs.badRows must beEmptyDir
+    }
+  }


can we have the two iab specs turned into one spec? No need to have two different files for the same enrichment.

BenFradet

One important q regarding anon ip and we're good to go I think 👍

BenFradet · 2018-06-05T14:26:53Z

3-enrich/config/enrichments/iab_spiders_and_robots_enrichment.json

+                "uri": "s3://my-private-bucket/iab"
+            },
+            "httpUsername": "USERNAME_PLACEHOLDER",
+            "httpPassword": "PASSWORD_PLACEHOLDER"


nit: I'd remove basic auth or change the uris to be http to make the example coherent

BenFradet · 2018-06-05T14:30:45Z

.../main/scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/EnrichmentManager.scala

+                         Option(event.derived_tstamp).map(EventEnrichments.fromTimestamp))
+          .map(_.some)
+      case None => None.success
+    }


One thing I missed before, at this point event.user_ipaddress may have been modified by anon ip, no?

Good point, I'll move this step to be executed just after the IP lookups enrichment - that shouldn't be affected by anonymization

BenFradet · 2018-06-05T14:48:48Z

...park-enrich/src/main/scala/com.snowplowanalytics.snowplow.enrich.spark/EnrichJobConfig.scala

+      case None => Nil
+    }
+    ipLookupFiles ++ iabFiles
+  }


I think this should be defined in the enrichment registry, I logged #3789. Of course, it's out of scope for your release.

BenFradet · 2018-06-05T14:50:09Z

...om.snowplowanalytics.snowplow.enrichments/iab_spiders_and_robots_enrichment/jsonschema/1-0-0

+        "parameters"
+    ],
+    "additionalProperties": false
+}


don't forget to rm

alexanderdean · 2018-06-05T22:29:45Z

Remember @rzats that once you get the green ticket, you need to start work on assembling the R107 PR (have you come up with an archeological site yet?)

BenFradet

I don't think the enrichment can work in stream enrich as it is.

Please try to test it before asking for a review, that will speed things up.

BenFradet · 2018-06-07T09:55:44Z

.../main/scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/EnrichmentManager.scala

+        unitSuccess
+      }
+      case f => f
+    }


this is just map, no?

BenFradet · 2018-06-07T09:58:26Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/registry/IabEnrichment.scala

+   *
+   * @param config The iab_spiders_and_robots_enrichment JSON
+   * @param name   The name of the lookup:
+   *               "geo", "isp", "organization", "domain"


BenFradet · 2018-06-07T10:01:44Z

...om.snowplowanalytics.snowplow.enrich.common/enrichments/registry/EnrichmentConfigsSpec.scala

+        "parameters": {
+          "ipFile": {
+            "database": "ip_exclude_current_cidr.txt",
+            "uri": "https://example.com"


that was meant to add a / at the end of uri to check uri resolving works properly

BenFradet · 2018-06-07T10:04:24Z

3-enrich/stream-enrich/project/Dependencies.scala

@@ -42,7 +42,7 @@ object Dependencies {
    val json4s               = "3.2.11"
    val pureconfig           = "0.8.0"
    val snowplowRawEvent     = "0.1.0"
-    val snowplowCommonEnrich = "0.32.0"
+    val snowplowCommonEnrich = "0.34.0"


I don't see this https://github.com/snowplow/snowplow/blob/master/3-enrich/stream-enrich/core/src/main/scala/com.snowplowanalytics.snowplow.enrich.stream/Enrich.scala#L216-L243 being modified.

Thanks for spotting - those changes were apparently lost after rebasing, will fix. I'll also be doing some smoke testing today which should help prevent issues like these

BenFradet · 2018-06-11T09:18:09Z

@rzats feel free to ping me once you've resolved the points I raised during the last review

…s & Bots List (close #937)

alexanderdean · 2018-06-11T22:08:25Z

What's the hold-up here @rzats ?

BenFradet · 2018-06-13T12:37:52Z

superseded by #3799, closing

rzats requested review from chuwy and BenFradet May 2, 2018 13:02

BenFradet reviewed May 3, 2018

View reviewed changes

rzats force-pushed the iab-enrichment branch from 87f5411 to 99ba84f Compare May 9, 2018 20:21

BenFradet reviewed May 10, 2018

View reviewed changes

rzats force-pushed the iab-enrichment branch from 99ba84f to 15b8369 Compare May 10, 2018 11:43

chuwy approved these changes May 10, 2018

View reviewed changes

BenFradet mentioned this pull request May 11, 2018

Beam Enrich: support enrichments relying on local files #3736

Closed

rzats force-pushed the iab-enrichment branch 5 times, most recently from 6ad4061 to 04784b5 Compare May 14, 2018 16:37

rzats force-pushed the iab-enrichment branch from 715185c to 08b4841 Compare May 28, 2018 09:39

BenFradet reviewed May 28, 2018

View reviewed changes

rzats force-pushed the iab-enrichment branch from 08b4841 to d57a58b Compare May 28, 2018 17:45

BenFradet reviewed May 29, 2018

View reviewed changes

rzats force-pushed the iab-enrichment branch 2 times, most recently from c5cd408 to 710bc9d Compare June 4, 2018 11:39

BenFradet mentioned this pull request Jun 5, 2018

Scala Common Enrich: make the list of files to cache available from the EnrichmentRegistry #3789

Closed

BenFradet reviewed Jun 5, 2018

View reviewed changes

knservis mentioned this pull request Jun 6, 2018

Release/r106 acropolis snowplow/snowplowanalytics.com#679

Merged

5 tasks

rzats force-pushed the iab-enrichment branch 3 times, most recently from 30db0b2 to f247ecd Compare June 6, 2018 20:49

BenFradet reviewed Jun 7, 2018

View reviewed changes

rzats added 8 commits June 11, 2018 20:17

Scala Common Enrich: add lookup based on IAB/ABC International Spider…

04a7216

…s & Bots List (close #937)

Scala Common Enrich: bump to 0.34.0 (close #3758)

bb0618b

Spark Enrich: bump scala-common-enrich to 0.34.0 (close #3729)

96fee36

Spark Enrich: bump to 1.15.0 (close #3728)

dec7559

Stream Enrich: bump scala-common-enrich to 0.34.0 (close #3730)

0eceddf

Stream Enrich: bump to 0.18.0 (close #3727)

ff50d92

Spark Enrich: add support for the IAB Enrichment (close #3772)

ffd9b81

Stream Enrich: add support for the IAB enrichment (close #xxxx)

8586726

rzats force-pushed the iab-enrichment branch 2 times, most recently from bf7fb0d to 8586726 Compare June 11, 2018 20:44

BenFradet closed this Jun 13, 2018

BenFradet deleted the iab-enrichment branch June 13, 2018 12:38

IAB Robots & Spiders enrichment #3750

IAB Robots & Spiders enrichment #3750

Conversation

rzats commented May 2, 2018 • edited

BenFradet left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderdean commented May 3, 2018

rzats commented May 9, 2018

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet commented May 10, 2018 • edited

chuwy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderdean commented Jun 5, 2018

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet commented Jun 11, 2018

alexanderdean commented Jun 11, 2018

BenFradet commented Jun 13, 2018

rzats commented May 2, 2018 •

edited

BenFradet commented May 10, 2018 •

edited