From 9c9a51c2e20da9366984eddf9ea15994b2d2d056 Mon Sep 17 00:00:00 2001 From: Clare Constantine Date: Wed, 8 Jan 2020 13:30:01 -0800 Subject: [PATCH 1/3] log share filtering errors to firehose instead of slog --- .../app/controllers/activities_controller.rb | 13 +++++++++---- .../helpers/profanity_privacy_helper.rb | 15 +++++++++++++-- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/dashboard/app/controllers/activities_controller.rb b/dashboard/app/controllers/activities_controller.rb index 24282c3bad2cc..9f2373390de23 100644 --- a/dashboard/app/controllers/activities_controller.rb +++ b/dashboard/app/controllers/activities_controller.rb @@ -1,5 +1,6 @@ require 'cdo/activity_constants' require 'cdo/share_filtering' +require 'cdo/firehose' class ActivitiesController < ApplicationController include LevelsHelper @@ -65,10 +66,14 @@ def milestone params[:program].strip_utf8mb4 ) if share_checking_error - slog( - tag: 'share_checking_error', - error: "#{share_checking_error.class.name}: #{share_checking_error}", - level_source_id: @level_source.id + FirehoseClient.instance.put_record( + study: 'share_filtering', + study_group: 'v0', + event: 'share_filtering_error', + data_string: "#{share_checking_error.class.name}: #{share_checking_error}", + data_json: { + level_source_id: @level_source.id + }.to_json ) end end diff --git a/shared/middleware/helpers/profanity_privacy_helper.rb b/shared/middleware/helpers/profanity_privacy_helper.rb index 7ffb64b93c84c..4b5965b2d4c3b 100644 --- a/shared/middleware/helpers/profanity_privacy_helper.rb +++ b/shared/middleware/helpers/profanity_privacy_helper.rb @@ -1,6 +1,7 @@ require 'cdo/share_filtering' require_relative './bucket_helper.rb' require_relative './source_bucket.rb' +require 'cdo/firehose' class ProfanityPrivacyError < StandardError def initialize(flagged_text) @@ -30,8 +31,18 @@ def channel_policy_violation?(channel_id) def title_profanity_privacy_violation(name, locale) share_failure = begin ShareFiltering.find_name_failure(name, locale) - rescue OpenURI::HTTPError, IO::EAGAINWaitReadable - # If WebPurify or Geocoder are unavailable, default to viewable + rescue OpenURI::HTTPError, IO::EAGAINWaitReadable => error + # If WebPurify or Geocoder are unavailable, default to viewable, but log error + FirehoseClient.instance.put_record( + study: 'share_filtering', + study_group: 'v0', + event: 'share_filtering_error', + data_string: "#{error.class.name}: #{error}", + data_json: { + name: name, + locale: locale + }.to_json + ) return false end share_failure From 14ac8bcd720b54123e42ec0d3abda114a23e5702 Mon Sep 17 00:00:00 2001 From: Clare Constantine Date: Wed, 8 Jan 2020 13:30:32 -0800 Subject: [PATCH 2/3] update tests --- .../controllers/activities_controller_test.rb | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/dashboard/test/controllers/activities_controller_test.rb b/dashboard/test/controllers/activities_controller_test.rb index 2ba4be48faae5..9f0b70e07f9e0 100644 --- a/dashboard/test/controllers/activities_controller_test.rb +++ b/dashboard/test/controllers/activities_controller_test.rb @@ -5,7 +5,15 @@ class ActivitiesControllerTest < ActionController::TestCase include LevelsHelper include UsersHelper + def stub_firehose + FirehoseClient.instance.stubs(:put_record).with do |args| + @firehose_record = args + true + end + end + setup do + stub_firehose client_state.reset Gatekeeper.clear @@ -899,13 +907,10 @@ def expect_controller_logs_milestone_regexp(regexp) assert_equal_expected_keys expected_response, JSON.parse(@response.body) end - test 'sharing program with http error slogs' do + test 'sharing program with http error logs' do # allow sharing when there's an error, slog so it's possible to look up and review later WebPurify.stubs(:find_potential_profanity).raises(OpenURI::HTTPError.new('something broke', 'fake io')) - @controller.expects(:slog).with(:tag, :error, :level_source_id) do |params| - params[:tag] == 'share_checking_error' && params[:error] == 'OpenURI::HTTPError: something broke' && !params[:level_source_id].nil? - end @controller.expects(:slog).with(:tag) {|params| params[:tag] == 'activity_finish'} assert_creates(LevelSource) do @@ -917,15 +922,17 @@ def expect_controller_logs_milestone_regexp(regexp) end assert_response :success + + assert @firehose_record[:study], 'share_filtering' + assert @firehose_record[:event], 'share_filtering_error' + assert @firehose_record[:data_string], 'OpenURI::HTTPError: something broke' + refute_nil @firehose_record[:data_json]["level_source_id"] end - test 'sharing program with IO::EAGAINWaitReadable error slogs' do + test 'sharing program with IO::EAGAINWaitReadable error logs' do WebPurify.stubs(:find_potential_profanity).raises(IO::EAGAINWaitReadable) # allow sharing when there's an error, slog so it's possible to look up and review later - @controller.expects(:slog).with(:tag, :error, :level_source_id) do |params| - params[:tag] == 'share_checking_error' && params[:error] == 'IO::EAGAINWaitReadable: Resource temporarily unavailable' && !params[:level_source_id].nil? - end @controller.expects(:slog).with(:tag) {|params| params[:tag] == 'activity_finish'} assert_creates(LevelSource) do @@ -937,6 +944,11 @@ def expect_controller_logs_milestone_regexp(regexp) end assert_response :success + + assert @firehose_record[:study], 'share_filtering' + assert @firehose_record[:event], 'share_filtering_error' + assert @firehose_record[:data_string], 'IO::EAGAINWaitReadable: Resource temporarily unavailable' + refute_nil @firehose_record[:data_json]["level_source_id"] end test 'sharing program with swear word in Spanish rejects word' do From 5539821e033eca84f7fba009395506edfdff9888 Mon Sep 17 00:00:00 2001 From: Clare Constantine Date: Wed, 8 Jan 2020 13:34:31 -0800 Subject: [PATCH 3/3] use consistent naming --- dashboard/app/controllers/activities_controller.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dashboard/app/controllers/activities_controller.rb b/dashboard/app/controllers/activities_controller.rb index 9f2373390de23..750f5ccc246a4 100644 --- a/dashboard/app/controllers/activities_controller.rb +++ b/dashboard/app/controllers/activities_controller.rb @@ -53,9 +53,9 @@ def milestone if @level.game.sharing_filtered? begin share_failure = ShareFiltering.find_share_failure(params[:program], locale) - rescue OpenURI::HTTPError, IO::EAGAINWaitReadable => share_checking_error + rescue OpenURI::HTTPError, IO::EAGAINWaitReadable => share_filtering_error # If WebPurify or Geocoder fail, the program will be allowed, and we - # retain the share_checking_error to log it alongside the level_source + # retain the share_filtering_error to log it alongside the level_source # ID below. end end @@ -65,12 +65,12 @@ def milestone @level, params[:program].strip_utf8mb4 ) - if share_checking_error + if share_filtering_error FirehoseClient.instance.put_record( study: 'share_filtering', study_group: 'v0', event: 'share_filtering_error', - data_string: "#{share_checking_error.class.name}: #{share_checking_error}", + data_string: "#{share_filtering_error.class.name}: #{share_filtering_error}", data_json: { level_source_id: @level_source.id }.to_json