Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Main Events schema to match event ping events #447

Merged
merged 1 commit into from
Jul 2, 2018

Conversation

sunahsuh
Copy link
Contributor

@sunahsuh sunahsuh commented Jul 2, 2018

As mentioned in mozilla/telemetry-streaming#149, this is the update to main ping events output to match the new schema from events ping output. The only really interesting part is the new session_start_time field, which matches the format of the session_start_time payload field in the events ping.

@sunahsuh sunahsuh requested a review from jklukas July 2, 2018 16:55
@jklukas
Copy link
Contributor

jklukas commented Jul 2, 2018

circleci is showing a compilation error:

[error] /telemetry-batch-view/src/test/scala/com/mozilla/telemetry/views/MainEventsViewTest.scala:82:75: integer number too large
[error]     events.select("session_start_time").collect.head.getLong(0) should be(1528394400000)

.select("document_id", "client_id", "normalized_channel", "country", "locale", "app_name", "app_version", "os",
"os_version", "e10s_enabled", "subsession_start_date", "subsession_length", "sync_configured",
"sync_count_desktop", "sync_count_mobile", "timestamp", "sample_id", "active_experiment_id",
"active_experiment_branch", "experiments", "events")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this select going away? Is it simply redundant with the exploded.selectExpr below? Is Spark smart enough to prune the unused columns early if we don't select down the column list here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically it shouldn't matter since this method only does transformations and the triggering action is the write in the calling method.

@@ -84,5 +78,7 @@ class MainEventsViewTest extends FlatSpec with Matchers with DataFrameSuiteBase
sampledEvents
.where("document_id = '72062950-3daf-450e-adfd-58eda3151a97'")
.count should be(1)

events.select("session_start_time").collect.head.getLong(0) should be(1528394400000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need an L suffix on the literal here.

@codecov-io
Copy link

codecov-io commented Jul 2, 2018

Codecov Report

Merging #447 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #447   +/-   ##
=======================================
  Coverage   69.85%   69.85%           
=======================================
  Files          45       45           
  Lines        4011     4011           
  Branches       83       81    -2     
=======================================
  Hits         2802     2802           
  Misses       1209     1209
Impacted Files Coverage Δ
...a/com/mozilla/telemetry/views/MainEventsView.scala 13.04% <100%> (ø) ⬆️

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 4c8316e...71a3a07. Read the comment docs.

@sunahsuh sunahsuh merged commit 82fd218 into mozilla:master Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants