r100-epidaurus #3599

knservis · 2018-01-29T14:59:01Z

TODO:

Update CHANGELOG with release date
Update CHANGELOG commits from Scala Common Enrich: add PII Enrichment #3588
Blog post Blog post R100 snowplowanalytics.com#497
Update wiki (changes are on the release/r100-epidaurus branch)
Update blogpost with release date
Update wiki with release date
Depending on what we do for Patch com.snowplowanalytics.snowplow.enrichments/pii_enrichment_config/jsonschema/1-0-0 iglu-central#721 we may need changes here too

Known issues:

For a specific invalid configuration scenario the code may create a new key pointing to an empty object. See: DocumentContext map creates new property under certain conditions json-path/JsonPath#438

alexanderdean · 2018-01-30T22:48:14Z

Hey @knservis - please add me as reviewer only when @chuwy has green-ticked. That will be my cue to get this deployed and tested.

alexanderdean · 2018-01-31T10:33:19Z

@chuwy is out today

chuwy

Hey @knservis,

So, all good for me. Though I have couple questions about scalafmt-commits.

Was it applied to to your PII Enrichment commit? It doesn't look so. If no then how does it work?
As already mentioned I don't like bringing all these formatting tools straight inside code-base: otherwise we'll end up with ten different tools for linting and formatting, each one is doing some overly automatic job. I'd like to keep those out of working tree and use it rather as "conflict resolver" rather than "chief editor". I'm not going to block on it, but please consider leaving only Scala Common Enrich: apply automated code formatting #3532

chuwy · 2018-02-01T17:33:00Z

3-enrich/scala-common-enrich/build.sbt

+    name              := "snowplow-common-enrich",
+    version           := "0.31.0-M1",
+    description       := "Common functionality for enriching raw Snowplow events",
+    scalafmtConfig    := file(".scalafmt.conf"),


Can we move this to separate set of settings, like BuildSettings.formatting.

chuwy · 2018-02-01T17:37:19Z

3-enrich/scala-common-enrich/project/plugins.sbt

@@ -1 +1,5 @@
 addSbtPlugin("me.lessis" % "bintray-sbt" % "0.3.0")
+
+addSbtPlugin("io.get-coursier" % "sbt-coursier" % "1.0.0-RC13")


Is coursier a part of scalafmt commit? Is it required?

There is some sort of bug in the plugin when using it with ivy. I could only get it to work with coursier.

can we use 1.0.1? It's been released since this commit.

chuwy · 2018-02-01T17:37:53Z

...-common-enrich/src/main/scala/com.snowplowanalytics.snowplow.enrich/common/EtlPipeline.scala

-  EnrichmentRegistry,
-  EnrichmentManager
-}
+import adapters.{AdapterRegistry,      RawEvent}


I'm still very hazy on these kind of indentations.

Agreed. In this instance the alignment is a bit weird. There are three options:

Change settings in scalafmt for all files

Switch off formatting locally there

In this particular case, move the import outputs.EnrichedEvent between the two multi imports so that scalafmt won't try to align them.

What implications has first option?

It will affect formatting in all files. If you want to experiment with it, just modify .scalafmt.conf on the root of SCE and run sbt clean compile. Then you can see the changes with git diff.

can we fix this in the end?

What is happening with this? I thought R100 was code complete...

this was fixed afaict

Hey, sorry i did not respond to that comment. I thought that this is all resolved (#3599 (comment)). Yes that was addressed along with all other formatting

chuwy · 2018-02-01T18:01:15Z

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

-trait Enrichment {
-  val version: DefaultArtifactVersion
-}
+trait Enrichment


So, as we deleted its single member, is there any other justification for its existence left?

I think it is useful as a "marker trait" enabling things like the EnrichmentMap here:

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

Line 65 in 362ebb5

type EnrichmentMap = Map[String, Enrichment]

but I may be wrong

Yeah, it seems to enable some kind of reflection-based machinery, but I don't see why it cannot be implemented in less boilerplate and reflection-less way: https://github.com/snowplow/snowplow/blob/master/3-enrich/scala-common-enrich/src/main/scala/com.snowplowanalytics.snowplow.enrich/common/enrichments/EnrichmentRegistry.scala#L330. Do you think it is worth to create an issue to explore?

Any opportunity to remove reflection is worth looking into. I created #3610

knservis · 2018-02-05T11:25:43Z

@chuwy

Yes it was used there.
I thought we wanted a single formatting tool to ensure formatting automatically. Lets talk about it.

BenFradet

Looks good 👍 , however I'm wondering if we couldn't make things more consistent regarding alignment.

Sometimes the formatted version feels weirder than the original.

BenFradet · 2018-02-12T15:23:59Z

3-enrich/scala-common-enrich/project/plugins.sbt

@@ -1 +1,5 @@
 addSbtPlugin("me.lessis" % "bintray-sbt" % "0.3.0")
+
+addSbtPlugin("io.get-coursier" % "sbt-coursier" % "1.0.0-RC13")


can we use 1.0.1? It's been released since this commit.

BenFradet · 2018-02-12T15:25:02Z

...-common-enrich/src/main/scala/com.snowplowanalytics.snowplow.enrich/common/EtlPipeline.scala

-  EnrichmentRegistry,
-  EnrichmentManager
-}
+import adapters.{AdapterRegistry,      RawEvent}


can we fix this in the end?

BenFradet · 2018-02-12T15:30:44Z

3-enrich/scala-common-enrich/.scalafmt.conf

@@ -0,0 +1,14 @@
+style = default
+align = some
+maxColumn = 140


can we reduce this so that it at least fits github number of columns (I believe it's 120) or even lower? that will ease reviews

@BenFradet I am not sure what the GH recommended max width is. 120 sounds good to me, although it will cause some reformatting (e.g. 3-enrich/scala-common-enrich/src/main/scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/UrbanAirshipAdapter.scala L53)

BenFradet · 2018-02-12T15:35:03Z

.../com.snowplowanalytics.snowplow.enrich/common/adapters/registry/GoogleAnalyticsAdapter.scala

@@ -494,7 +512,7 @@ object GoogleAnalyticsAdapter extends Adapter {
   * @return a translated params
   */
  private def translatePayload(
-    originalParams: Map[String, String],
+    originalParams: Map[String,   String],


can we exclude this type of alignment? doesn't make a lot of sense to me, if we can't that's fine...

BenFradet · 2018-02-12T15:39:48Z

.../scala/com.snowplowanalytics.snowplow.enrich/common/adapters/registry/MailchimpAdapter.scala

+      case (None,       _) => s"Request body is empty: no ${VendorName} event to process".failNel
+      case (_,          None) => s"Request body provided but content type empty, expected ${ContentType} for ${VendorName}".failNel
+      case (_,          Some(ct)) if ct != ContentType => s"Content type of ${ct} provided, expected ${ContentType} for ${VendorName}".failNel
+      case (Some(body), _) => {


I don't understand why sometime => are aligned and sometimes members of the patmat are aligned, can we prioritize one over the others?

BenFradet · 2018-02-13T18:20:52Z

fomatting looks nice 👍

BenFradet · 2018-02-21T09:19:16Z

@knservis afaict #3598 is missing from the commits and #3604 is missing from the changelog entries

knservis · 2018-02-21T12:45:55Z

@BenFradet Fixed both

knservis · 2018-02-21T14:58:00Z

...owplowanalytics.snowplow.enrich/common/enrichments/registry/PiiPseudonymizerEnrichment.scala

+  override def scramble(clearText: String): String = hash(clearText)
+  def hash(text: String): String = {
+    val bytes = hashFunction.digest(text.getBytes(TextEncoding))
+    String.format("%0" + bytes.size*2 + "x", new java.math.BigInteger(1, bytes))


@chuwy What do you think about this formatting string? The objective is to keep the output at constant length. The byte array should have always the same length for the same algorithm.

Yes, it looks good to me. Though I don't think I'm aware of all implications it has.

BenFradet · 2018-02-22T10:41:13Z

@knservis can you add an example pii enrichment in https://github.com/snowplow/snowplow/tree/master/3-enrich/config/enrichments as part of #3472 ?

BenFradet · 2018-02-22T10:54:42Z

and also reorder the commits so that Prepare for release is last both in order and chronologically.

BenFradet · 2018-02-22T13:44:13Z

it's because gh orders commits according to their timestamps

…ymization (closes #3528)

…closes #3541)

BenFradet · 2018-02-22T16:16:52Z

Also how do we deal with:

For a specific invalid configuration scenario the code may create a new key pointing to an empty object. See: json-path/JsonPath#438

knservis · 2018-02-22T17:25:39Z

@BenFradet According to @alexanderdean this is not a showstopper and a very unlikely case. I could fix it on jayway probably.

alexanderdean · 2018-02-22T18:36:50Z

Let's create a bug issue to track this edge case

BenFradet · 2018-02-23T12:13:00Z

created #3636

alexanderdean · 2018-02-26T14:31:37Z

Exciting!

knservis added this to the R100 Epidaurus (PII Enrichment phase 1) milestone Jan 29, 2018

knservis force-pushed the release/gdpr-1 branch from 9f85150 to 0676b3b Compare January 30, 2018 13:37

knservis requested review from alexanderdean and chuwy January 30, 2018 13:40

knservis force-pushed the release/gdpr-1 branch 2 times, most recently from 02eeb71 to 95c9e16 Compare January 30, 2018 14:49

alexanderdean removed their request for review January 30, 2018 22:47

knservis force-pushed the release/gdpr-1 branch from 95c9e16 to b2974d8 Compare January 31, 2018 14:17

chuwy approved these changes Feb 1, 2018

View reviewed changes

knservis requested a review from alexanderdean February 5, 2018 09:56

knservis force-pushed the release/gdpr-1 branch from b2974d8 to f9b9851 Compare February 5, 2018 10:43

knservis mentioned this pull request Feb 5, 2018

Scala Common Enrich: remove the Enrichment trait #3610

Closed

knservis force-pushed the release/gdpr-1 branch from f9b9851 to 7bd87dd Compare February 7, 2018 16:47

BenFradet approved these changes Feb 12, 2018

View reviewed changes

knservis force-pushed the release/gdpr-1 branch 2 times, most recently from ff0e03f to 5224015 Compare February 15, 2018 08:41

knservis force-pushed the release/gdpr-1 branch from 5224015 to 5d16f1f Compare February 21, 2018 12:45

knservis force-pushed the release/gdpr-1 branch 3 times, most recently from 65e7e25 to 15138ab Compare February 21, 2018 14:56

knservis commented Feb 21, 2018

View reviewed changes

knservis force-pushed the release/gdpr-1 branch from 15138ab to c36fb68 Compare February 21, 2018 15:14

knservis force-pushed the release/gdpr-1 branch 2 times, most recently from daaa656 to 354e8a3 Compare February 22, 2018 13:35

knservis force-pushed the release/gdpr-1 branch 2 times, most recently from e16671a to 082ee77 Compare February 22, 2018 14:58

knservis added 3 commits February 22, 2018 17:00

Redshift: widen se_label to 4,096 to support URLs etc (closes #196)

f22a959

Redshift: widen sensitive columns in atomic.events to support pseudon…

4bb64ac

…ymization (closes #3528)

Scala Common Enrich: remove unused version member in Enrichment trait (…

38bc2b9

…closes #3541)

knservis force-pushed the release/gdpr-1 branch from 082ee77 to 54dbe08 Compare February 22, 2018 15:20

knservis force-pushed the release/gdpr-1 branch 2 times, most recently from 39e352b to 4af8550 Compare February 23, 2018 16:38

knservis added 9 commits February 26, 2018 11:44

Scala Common Enrich: bump commons-codec to 1.11 (closes #3638)

a152418

Scala Common Enrich: add PII Enrichment (closes #3472)

eb16081

Stream Enrich: bump scala-common-enrich to 0.31.0 (closes #3597)

05c6af2

Stream Enrich: bump to 0.14.0 (closes #3596)

e5362d8

Stream Enrich: use generated Settings for version in test (closes #3604)

539e79c

Scala Common Enrich: use automated code formatting (closes #3496)

519956e

Scala Common Enrich: apply automated code formatting (closes #3532)

b2e6eee

Scala Common Enrich: bump to 0.31.0 (closes #3598)

9b34a4c

Prepare for release

9c8b5c3

knservis force-pushed the release/gdpr-1 branch 2 times, most recently from fe750c1 to 9c8b5c3 Compare February 26, 2018 12:05

BenFradet merged commit a1bf092 into master Feb 26, 2018

BenFradet mentioned this pull request Feb 26, 2018

Stream Enrich: use generated Settings for version in test #3605

Closed

BenFradet deleted the release/gdpr-1 branch April 30, 2018 10:10

		@@ -1 +1,5 @@
		addSbtPlugin("me.lessis" % "bintray-sbt" % "0.3.0")

		addSbtPlugin("io.get-coursier" % "sbt-coursier" % "1.0.0-RC13")

r100-epidaurus #3599

r100-epidaurus #3599

Conversation

knservis commented Jan 29, 2018 • edited

alexanderdean commented Jan 30, 2018

alexanderdean commented Jan 31, 2018

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

knservis Feb 5, 2018 • edited

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

knservis commented Feb 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

Choose a reason for hiding this comment

BenFradet commented Feb 13, 2018

BenFradet commented Feb 21, 2018

knservis commented Feb 21, 2018

knservis Feb 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet commented Feb 22, 2018

BenFradet commented Feb 22, 2018

BenFradet commented Feb 22, 2018

BenFradet commented Feb 22, 2018

knservis commented Feb 22, 2018

alexanderdean commented Feb 22, 2018

BenFradet commented Feb 23, 2018

alexanderdean commented Feb 26, 2018

knservis commented Jan 29, 2018 •

edited

knservis Feb 5, 2018 •

edited

knservis Feb 21, 2018 •

edited