Add case classes representing crash and main pings. by maurodoglio · Pull Request #41 · mozilla/telemetry-streaming

maurodoglio · 2017-05-11T16:38:07Z

This PR introduces a few case classes representing crash and main pings.
This classes are then used to extract ping data out of HekaMessages
Fixes #39

codecov-io · 2017-05-11T16:45:39Z

Codecov Report

Merging #41 into master will increase coverage by 8.39%.
The diff coverage is 92.66%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage      60%   68.39%   +8.39%     
==========================================
  Files           2        3       +1     
  Lines         170      231      +61     
  Branches        7       12       +5     
==========================================
+ Hits          102      158      +56     
- Misses         68       73       +5

Impacted Files	Coverage Δ
...ala/com/mozilla/telemetry/timeseries/package.scala	`88.46% <100%> (+0.96%)`	⬆️
...in/scala/com/mozilla/telemetry/pings/package.scala	`85% <85%> (ø)`
.../mozilla/telemetry/streaming/ErrorAggregator.scala	`61.21% <97.01%> (+5.73%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7fc30e...82d28ba. Read the comment docs.

fbertsch · 2017-05-11T17:29:56Z

Per bug 1334457: What about moving these case classes to moztelemetry, where we can import them into either t-b-v, here, or (eventually) other projects? Could then start to move other case classes there as well.

vitillo · 2017-05-12T13:10:57Z

+import org.json4s._
+import org.json4s.jackson.JsonMethods.parse
+
+package object pings {


Let's move this to the moztelemetry library as suggested by Frank.

While I like the principle, I think this would be a pain to handle in practice for 2 reasons:
1- Changes to a central ping declaration (think of a new non-optional field) will require changes in every place where that ping is used, across different repositories.
2- Different projects may need different parts of a ping. For example in this patch I kept the definition of the pings intentionally loose. This made it easier to create test fixtures and it likely keeps the memory footprint at a minimum (not too sure about this though).
That said, this is the second time that I spend time defining the structure of a crash ping as a case class, I can see the benefits of moving this to moztelemetry.
@vitillo @fbertsch final thoughts?

vitillo · 2017-05-12T13:12:47Z

+
+  private def parseCrashPing(ping: CrashPing): Tuple1[Row] = {
+    // Non-main crashes are already retrieved from main pings
+    if(!ping.isMain()) return Tuple1(null)


How can a crash ping be a main ping?

I should rename that method to isMainCrash, it's a bit confusing with MainPing around

vitillo · 2017-05-12T13:15:37Z

+    dimensions.build
+  }
+
+  private def parseCrashPing(ping: CrashPing): Tuple1[Row] = {


Can we move this functionality within the CrashPing class itself?

This function is very specific to ErrorAggregator, the value it returns is coupled to the output schema.

Right, but you could create a subclass or use the "pimp my library" pattern.

I swear I was going to propose that. I love to pimp my classes 😎

vitillo · 2017-05-12T13:15:50Z

+    Tuple1(RowBuilder.merge(dimensions, stats.build))
+  }
+
+  private def parseMainPing(ping: MainPing): Tuple1[Row] = {


Can we move this functionality within the MainPing class itself?

maurodoglio · 2017-05-12T15:19:42Z

RFOL

vitillo · 2017-05-15T15:12:02Z

+                        // Environment is omitted it's partially available under meta
+                        meta: Meta
+                      ){
+


Please remove the newline. We should add a linter to travis to have an uniform style.

Activated scalastyle and fixed the problem

vitillo · 2017-05-15T15:12:41Z

+                        meta: Meta
+                      ){
+
+    def isMainCrash(): Boolean = {


Please add another method for content process crashes.

I'm not sure it's needed because we are only taking main crashes coming from crash pings. Everything else is filtered out to avoid double counting.

vitillo · 2017-05-15T15:13:58Z

+    }
+
+    def timestamp(): Timestamp = {
+      new Timestamp(this.meta.Timestamp / 1000000)


Please add a comment specifying the timestamp resolution.

I moved this method to pings.Meta and added a comment

vitillo · 2017-05-15T15:15:11Z

+                       // Environment omitted because it's mostly available under meta
+                       meta: Meta
+                     ){
+    def getCountHistogramValue(histogram_name: String): Int = {


Please add an assert to verify that the desired histogram is in fact a count histogram. The same comment applies elsewhere in this class.

vitillo · 2017-05-15T15:17:56Z

+          case JInt(count) => count.toInt
+          case _ => 0
+        }
+      } catch { case _: Throwable => 0 }


A missing histogram should not generate a value of 0 but something equivalent to "NA" (e.g. null), as the semantics is quite different. The same comment applies elsewhere in this class.

I changed these methods to return an Option

vitillo · 2017-05-15T15:24:35Z

+
+  case class OS(
+                 name: Option[String],
+                 version: Option[String])


The formatting of case classes is inconsistent. E.g. sometimes the closing parentheses is right next to the last field while other times it isn't. Please use a linting tool.

I reformatted these classes following the scala style guidelines. I also activated scalastyle, but unfortunately it doesn't provide any linting for this kind of problems.

vitillo · 2017-05-15T15:25:14Z

+                 name: Option[String],
+                 version: Option[String])
+
+  def messageToCrashPing(message: Message): CrashPing = {


Can we make this method a constructor?

vitillo · 2017-05-15T15:25:22Z

+    ping.extract[CrashPing]
+  }
+
+  def messageToMainPing(message: Message): MainPing = {


Can we make this method a constructor?

vitillo · 2017-05-15T15:27:58Z

+    dimensions("os_version") = meta.`environment.system`.map(_.os.version)
+    dimensions("architecture") = meta.`environment.build`.flatMap(_.architecture)
+    dimensions("country") = Some(meta.geoCountry)
+    dimensions("experiment_id") = for {


This works for old style telemetry experiments but Quantum experiments are going to use shield, which supports multiple experiments. @sunahsuh can comment on the right fields to use.

Talked to @maurodoglio offline but for posterity, the docs for the new experiment block is here: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/environment.html#experiments

vitillo · 2017-05-15T15:29:15Z

+    dimensions.build
+  }
+
+  class ParsableCrashPing(ping: CrashPing) {


I don't think this is a good name for this class.

Changed to ErrorAggregatorCrashPing. I couldn't think of anything shorter and meaningful.

maurodoglio · 2017-05-17T14:16:04Z

I think I addressed all the issues. @vitillo this is RFAL!

vitillo

LGTM. Please verify that the aggregates match the ones we are currently generating.

maurodoglio · 2017-06-05T13:25:11Z

Apparently this version of the job is not producing any data, neither in streaming nor in batch mode. I'm debugging it.

maurodoglio · 2017-06-05T16:52:17Z

I'll rebase --autosquash before merging

This also increases the test coverage adding some tests for the MainPing methods.

This gives us the ability to eventually return more than one Row per ping.

maurodoglio changed the title ~~This PR introduces a few case classes representing crash and main pings. This classes are then used to extract ping data out of HekaMessages~~ This PR introduces a few case classes representing crash and main pings. May 11, 2017

maurodoglio changed the title ~~This PR introduces a few case classes representing crash and main pings.~~ Add case classes representing crash and main pings. May 11, 2017

maurodoglio requested a review from vitillo May 11, 2017 16:39

maurodoglio mentioned this pull request May 11, 2017

Use case classes to parse crash and main pings #40

Closed

vitillo suggested changes May 12, 2017

View reviewed changes

maurodoglio force-pushed the use-case-classes branch from a8ab146 to 1eb966a Compare May 12, 2017 14:45

vitillo suggested changes May 15, 2017

View reviewed changes

maurodoglio force-pushed the use-case-classes branch from 3fa6704 to e4e92e2 Compare May 17, 2017 14:13

maurodoglio added the review label May 19, 2017

vitillo approved these changes May 22, 2017

View reviewed changes

Add case classes for main and crash pings

4de6135

maurodoglio force-pushed the use-case-classes branch from e4e92e2 to f81ab09 Compare May 26, 2017 11:02

maurodoglio self-assigned this Jun 6, 2017

Mauro Doglio added 11 commits June 6, 2017 14:40

Use case classes to parse pings out of heka messages

3cb7cba

Add non-main crashes to stats

9d7d83a

This also increases the test coverage adding some tests for the MainPing methods.

Decorate MainPings and CrashPings with a parse() method

3e2bd03

Rename CrashPing method isMain to isMainCrash

b503966

Add e10s, experiments and gfx dimensions to ErrorAggregator

49646ea

Add sbt alias command for test and linter runs

697630c

Add MPLv2 header to source code

f87cea6

Refactor Timestamp extraction

90779c8

Return None instead of zeros when values are NA

3241f2b

Wrap Row objects in Arrays instead of Tuple1

29da280

This gives us the ability to eventually return more than one Row per ping.

Better name Parsable[Main|Crash]Ping

8f2ed97

Mauro Doglio added 5 commits June 6, 2017 14:44

Minor style fixes

deb1edf

Fix failing tests and factor out scalar values

2ca6dbe

Standardize case classes indentation style

dd028aa

Add support for new-style experiments

7921835

Make messageTo[Crash|Main]Ping constructors

82d28ba

maurodoglio force-pushed the use-case-classes branch from 8313651 to 82d28ba Compare June 6, 2017 13:47

maurodoglio merged commit ba394a7 into master Jun 6, 2017

fbertsch mentioned this pull request Oct 18, 2017

Stream events to amplitude #80

Merged

Conversation

maurodoglio commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fbertsch commented May 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitillo May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurodoglio commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maurodoglio commented May 17, 2017

Uh oh!

vitillo left a comment

Choose a reason for hiding this comment

Uh oh!

maurodoglio commented Jun 5, 2017

Uh oh!

maurodoglio commented Jun 5, 2017

Uh oh!

Reviewers

maurodoglio commented May 11, 2017 •

edited

Loading

codecov-io commented May 11, 2017 •

edited

Loading

vitillo May 12, 2017 •

edited

Loading