From 8ef611f879b41b8a98b28aa82a6aa8b6faf6ca15 Mon Sep 17 00:00:00 2001 From: Andy Zhao Date: Thu, 11 Apr 2019 10:49:35 -0400 Subject: [PATCH] Remove Honeycomb from the app (#2366) * Remove Honeycomb related gems * Remove Honeycomb related code from controllers * Remove Honeycomb from RSS Reader * Remove Honeycomb related files * Remove Honeycomb related tests * Remove leftover Honeycomb methods --- Gemfile | 1 - Gemfile.lock | 15 -- app/controllers/application_controller.rb | 1 - app/controllers/chat_channels_controller.rb | 6 - app/controllers/comments_controller.rb | 3 - app/controllers/events_controller.rb | 1 - app/controllers/follows_controller.rb | 1 - app/controllers/reactions_controller.rb | 3 - app/controllers/stories_controller.rb | 4 - app/controllers/users_controller.rb | 2 +- app/services/rss_reader.rb | 160 +++++--------------- config/initializers/shutdown.rb | 4 - lib/instrumentation.rb | 26 ---- spec/services/rss_reader_spec.rb | 48 ------ 14 files changed, 35 insertions(+), 240 deletions(-) delete mode 100644 config/initializers/shutdown.rb delete mode 100644 lib/instrumentation.rb diff --git a/Gemfile b/Gemfile index 9ac50afa0..56984cf86 100644 --- a/Gemfile +++ b/Gemfile @@ -52,7 +52,6 @@ gem "front_matter_parser", "~> 0.2" gem "gemoji", "~> 3.0.0" gem "gibbon", "~> 3.2" gem "google-api-client", "~> 0.27" -gem "honeycomb-beeline", "~> 0.7.0" gem "html_truncator", "~> 0.4" gem "httparty", "~> 0.16" gem "inline_svg", "~> 1.3" diff --git a/Gemfile.lock b/Gemfile.lock index 3fc6130fd..1459648e0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -59,8 +59,6 @@ GEM activemodel (= 5.2.3) activesupport (= 5.2.3) arel (>= 9.0) - activerecord-honeycomb (0.4.0) - libhoney activestorage (5.2.3) actionpack (= 5.2.3) activerecord (= 5.2.3) @@ -334,8 +332,6 @@ GEM i18n (>= 0.7) faraday (0.15.4) multipart-post (>= 1.2, < 3) - faraday-honeycomb (0.3.1) - libhoney (>= 1.5.0) faraday_middleware (0.13.1) faraday (>= 0.7.4, < 1.0) fastimage (2.1.1) @@ -550,11 +546,6 @@ GEM hashie (3.6.0) heapy (0.1.4) hkdf (0.3.0) - honeycomb-beeline (0.7.0) - activerecord-honeycomb (>= 0.4.0) - faraday-honeycomb (>= 0.3.0) - libhoney (>= 1.8.1) - rack-honeycomb (>= 0.4.0) html_tokenizer (0.0.7) html_truncator (0.4.2) nokogiri (~> 1.5) @@ -609,9 +600,6 @@ GEM kaminari-core (1.1.1) launchy (2.4.3) addressable (~> 2.3) - libhoney (1.12.1) - addressable (~> 2.0) - http (>= 2.0, < 5.0) liquid (4.0.3) listen (3.1.5) rb-fsevent (~> 0.9, >= 0.9.4) @@ -713,8 +701,6 @@ GEM pusher-signature (0.1.8) raabro (1.1.6) rack (2.0.7) - rack-honeycomb (0.5.0) - libhoney (>= 1.5.0) rack-host-redirect (1.3.0) rack rack-protection (2.0.5) @@ -1044,7 +1030,6 @@ DEPENDENCIES guard (~> 2.15) guard-livereload (~> 2.5) guard-rspec (~> 4.7) - honeycomb-beeline (~> 0.7.0) html_truncator (~> 0.4) httparty (~> 0.16) inline_svg (~> 1.3) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 20a5048f7..b2baaf6da 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -2,7 +2,6 @@ class ApplicationController < ActionController::Base protect_from_forgery with: :exception, prepend: true include Pundit - include Instrumentation def require_http_auth authenticate_or_request_with_http_basic do |username, password| diff --git a/app/controllers/chat_channels_controller.rb b/app/controllers/chat_channels_controller.rb index 1c3497eef..5d83115c8 100644 --- a/app/controllers/chat_channels_controller.rb +++ b/app/controllers/chat_channels_controller.rb @@ -3,7 +3,6 @@ class ChatChannelsController < ApplicationController after_action :verify_authorized def index - add_param_context(:state) if params[:state] == "unopened" authorize ChatChannel render_unopened_json_response @@ -19,13 +18,11 @@ class ChatChannelsController < ApplicationController def show @chat_channel = ChatChannel.find_by(id: params[:id]) || not_found authorize @chat_channel - add_context(chat_channel_id: @chat_channel.id) end def create authorize ChatChannel @chat_channel = ChatChannelCreationService.new(current_user, params[:chat_channel]).create - add_context(chat_channel_id: @chat_channel.id) if @chat_channel.valid? render json: { status: "success", chat_channel: @chat_channel.to_json(only: %i[channel_name slug]) }, @@ -38,7 +35,6 @@ class ChatChannelsController < ApplicationController def update @chat_channel = ChatChannel.find(params[:id]) authorize @chat_channel - add_context(chat_channel_id: @chat_channel.id) ChatChannelUpdateService.new(@chat_channel, chat_channel_params).update if @chat_channel.valid? render json: { status: "success", @@ -52,7 +48,6 @@ class ChatChannelsController < ApplicationController def open @chat_channel = ChatChannel.find(params[:id]) authorize @chat_channel - add_context(chat_channel_id: @chat_channel.id) membership = @chat_channel.chat_channel_memberships.where(user_id: current_user.id).first membership.update(last_opened_at: 1.second.from_now, has_unopened_messages: false) @chat_channel.index! @@ -62,7 +57,6 @@ class ChatChannelsController < ApplicationController def moderate @chat_channel = ChatChannel.find(params[:id]) authorize @chat_channel - add_context(chat_channel_id: @chat_channel.id) command = chat_channel_params[:command].split case command[0] when "/ban" diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 3fe4a15b0..f089aeeb8 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -56,9 +56,6 @@ class CommentsController < ApplicationController @comment = Comment.new(permitted_attributes(Comment)) @comment.user_id = current_user.id - add_context(commentable_id: @comment.commentable_id, - commentable_type: @comment.commentable_type) - if @comment.save current_user.update(checked_code_of_conduct: true) if params[:checked_code_of_conduct].present? && !current_user.checked_code_of_conduct diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index 1bde6ea37..94358b0ea 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -11,6 +11,5 @@ class EventsController < ApplicationController def show @event = Event.find_by(slug: params[:id]) - add_context(event_id: @event.id) end end diff --git a/app/controllers/follows_controller.rb b/app/controllers/follows_controller.rb index f2f55d6ad..5578c8ed2 100644 --- a/app/controllers/follows_controller.rb +++ b/app/controllers/follows_controller.rb @@ -28,7 +28,6 @@ class FollowsController < ApplicationController else User.find(params[:followable_id]) end - add_param_context(:followable_type, :followable_id, :verb) need_notification = Follow.need_new_follower_notification_for?(followable.class.name) @result = if params[:verb] == "unfollow" follow = current_user.stop_following(followable) diff --git a/app/controllers/reactions_controller.rb b/app/controllers/reactions_controller.rb index d5163f2a8..64eff2cf6 100644 --- a/app/controllers/reactions_controller.rb +++ b/app/controllers/reactions_controller.rb @@ -4,7 +4,6 @@ class ReactionsController < ApplicationController def index skip_authorization - add_param_context(:article_id) if params[:article_id] id = params[:article_id] reactions = if efficient_current_user_id.present? @@ -22,7 +21,6 @@ class ReactionsController < ApplicationController reactions: reactions }.to_json else - add_param_context(:commentable_id, :commentable_type) comments = Comment.where( commentable_id: params[:commentable_id], commentable_type: params[:commentable_type], @@ -42,7 +40,6 @@ class ReactionsController < ApplicationController def create authorize Reaction - add_param_context(:reactable_id, :reactable_type, :category) Rails.cache.delete "count_for_reactable-#{params[:reactable_type]}-#{params[:reactable_id]}" category = params[:category] || "like" reaction = Reaction.where( diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 41da14266..5098a9f8a 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -3,7 +3,6 @@ class StoriesController < ApplicationController before_action :set_cache_control_headers, only: %i[index search show] def index - add_param_context(:username, :tag) return handle_user_or_organization_or_podcast_index if params[:username] return handle_tag_index if params[:tag] @@ -19,7 +18,6 @@ class StoriesController < ApplicationController def show @story_show = true - add_param_context(:username, :slug) if (@article = Article.find_by(path: "/#{params[:username].downcase}/#{params[:slug]}")&.decorate) handle_article_show elsif (@article = Article.find_by(slug: params[:slug])&.decorate) @@ -82,7 +80,6 @@ class StoriesController < ApplicationController @page = (params[:page] || 1).to_i @tag_model = Tag.find_by(name: @tag) || not_found @moderators = User.with_role(:tag_moderator, @tag_model) - add_param_context(:tag, :page) if @tag_model.alias_for.present? redirect_to "/t/#{@tag_model.alias_for}" return @@ -106,7 +103,6 @@ class StoriesController < ApplicationController @page = (params[:page] || 1).to_i num_articles = 35 @stories = article_finder(num_articles) - add_param_context(:page, :timeframe) if %w[week month year infinity].include?(params[:timeframe]) @stories = @stories.where("published_at > ?", Timeframer.new(params[:timeframe]).datetime). order("score DESC") diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 82d4fa6c5..5025e0021 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -23,7 +23,7 @@ class UsersController < ApplicationController authorize @user # raise permitted_attributes(@user).to_s if @user.update(permitted_attributes(@user)) - RssReader.new(request.request_id).delay.fetch_user(@user) if @user.feed_url.present? + RssReader.new.delay.fetch_user(@user) if @user.feed_url.present? notice = "Your profile was successfully updated." if @user.export_requested? notice += " The export will be emailed to you shortly." diff --git a/app/services/rss_reader.rb b/app/services/rss_reader.rb index 41298fb9b..a7a467636 100644 --- a/app/services/rss_reader.rb +++ b/app/services/rss_reader.rb @@ -3,10 +3,6 @@ class RssReader new.get_all_articles(force) end - def initialize(request_id = nil) - @request_id = request_id - end - def get_all_articles(force = true) User.where.not(feed_url: [nil, ""]).find_each do |user| # unless forced, fetch sparingly @@ -17,10 +13,6 @@ class RssReader end def fetch_user(user) - # add request_id to thread current variables to aid Honeycomb instrumentation - Thread.current[:request_id] = request_id - Thread.current[:span_id] = request_id - create_articles_for_user(user) end @@ -35,32 +27,32 @@ class RssReader attr_reader :request_id def create_articles_for_user(user) - with_span("create_articles_for_user", user_id: user.id, username: user.username) do |metadata| - user.update_column(:feed_fetched_at, Time.current) - feed = fetch_rss(user.feed_url.strip) + user.update_column(:feed_fetched_at, Time.current) + feed = fetch_rss(user.feed_url.strip) - metadata[:feed_length] = feed.entries.length if feed&.entries - - feed.entries.reverse_each do |item| - make_from_rss_item(item, user, feed) - rescue StandardError => e - log_error( - "RssReaderError: occurred while creating article", + feed.entries.reverse_each do |item| + make_from_rss_item(item, user, feed) + rescue StandardError => e + log_error( + "RssReaderError: occurred while creating article", + rss_reader_info: { user: user.username, feed_url: user.feed_url, item_count: get_item_count_error(feed), - error: e, - ) - end - rescue StandardError => e - log_error( - "RssReaderError: occurred while fetching feed", + error: e + }, + ) + end + rescue StandardError => e + log_error( + "RssReaderError: occurred while fetching feed", + rss_reader_info: { user: user.username, feed_url: user.feed_url, item_count: get_item_count_error(feed), - error: e, - ) - end + error: e + }, + ) end def get_item_count_error(feed) @@ -72,41 +64,25 @@ class RssReader end def fetch_rss(url) - with_span("fetch_rss", url: url) do |metadata| - xml = with_timer("http_get", metadata) do - HTTParty.get(url).body - end - with_timer("parse_xml", metadata) do - Feedjira::Feed.parse xml - end - end + xml = HTTParty.get(url).body + Feedjira::Feed.parse xml end def make_from_rss_item(item, user, feed) - with_span( - "make_from_rss_item", - item_id: item.entry_id, - item_title: item.title, - item_summary_size: item.summary&.size, - ) do |metadata| + return if medium_reply?(item) || article_exists?(user, item) - return if medium_reply?(item) || article_exists?(user, item) + feed_source_url = item.url.strip.split("?source=")[0] + article = Article.create!( + feed_source_url: feed_source_url, + user_id: user.id, + published_at: item.published, + published_from_feed: true, + show_comments: true, + body_markdown: RssReader::Assembler.call(item, user, feed, feed_source_url), + organization_id: user.organization_id.presence, + ) - article = with_timer("save_article", metadata) do - feed_source_url = item.url.strip.split("?source=")[0] - Article.create!( - feed_source_url: feed_source_url, - user_id: user.id, - published_at: item.published, - published_from_feed: true, - show_comments: true, - body_markdown: RssReader::Assembler.call(item, user, feed, feed_source_url), - organization_id: user.organization_id.presence, - ) - end - - send_slack_notification(article) - end + send_slack_notification(article) end def get_host_without_www(url) @@ -147,74 +123,6 @@ class RssReader end def log_error(error_msg, metadata) - # giving the error message a resemblance of structured logging - parts = metadata.map { |k, v| [k, v].join("=") } - parts = parts.unshift("#{error_msg}:") - Rails.logger.error(parts.join(" ")) - - # log error to Honeycomb if initialized - return unless Honeycomb.client - - ev = Honeycomb.client.event - ev.add(metadata) - ev.add_field("error_msg", error_msg) - ev.add_field("trace.trace_id", request_id) - ev.add_field("trace.parent_id", Thread.current[:span_id] || request_id) - ev.add_field("trace.span_id", SecureRandom.uuid) - ev.send - end - - # NOTE: honeycomb-beeline makes "with_span" theoretically obsolete, - # unfortunately there's a bug that prevents the gem to add fields after a - # a span has been initiated, so for now we'll keep this code in. - # see - # This wrapper takes a span name, some optional metadata, and a block; then - # emits a "span" to Honeycomb as part of the trace begun in the RequestTracer - # middleware. - # - # The special sauce in this method is the definition / resetting of thread - # local variables in order to correctly propagate "parent" identifiers down - # into the block. - def with_span(name, metadata = nil) - trace_id = Thread.current[:request_id] - return yield({}) unless trace_id && Honeycomb.client - - current_span_id = SecureRandom.uuid - start = Time.current - data = { - name: name, - "trace.span_id": current_span_id, - "trace.trace_id": trace_id, - service_name: "rss_reader" - } - # Capture the calling scope's span ID, then restore it at the end of the - # method. - parent_id = Thread.current[:span_id] - data["trace.parent_id"] = parent_id if parent_id - - # Set the current span ID before invoking the provided block, then capture - # the return value to return after emitting the Honeycomb event. - Thread.current[:span_id] = span_id - ret = yield data - - data[:duration_ms] = (Time.current - start) * 1000 - data.merge!(metadata) if metadata - - # log event to Honeycomb - ev = Honeycomb.client.event - ev.timestamp = start - ev.add(data) - ev.send - - ret - ensure - Thread.current[:span_id] = parent_id - end - - def with_timer(name, data) - start = Time.current - ret = yield - data[name + "_dur_ms"] = (Time.current - start) * 1000 if data - ret + Rails.logger.error(error_msg, metadata) end end diff --git a/config/initializers/shutdown.rb b/config/initializers/shutdown.rb deleted file mode 100644 index 59c0d78aa..000000000 --- a/config/initializers/shutdown.rb +++ /dev/null @@ -1,4 +0,0 @@ -# adding an exit hook to properly shutdown/close third party clients -at_exit do - Honeycomb.shutdown if defined?(Honeycomb) && Honeycomb.respond_to?(:shutdown) -end diff --git a/lib/instrumentation.rb b/lib/instrumentation.rb deleted file mode 100644 index a433e77d4..000000000 --- a/lib/instrumentation.rb +++ /dev/null @@ -1,26 +0,0 @@ -require "rack/honeycomb/middleware" - -module Instrumentation - def self.included(base) - base.before_action :add_user_info_to_honeycomb_event - end - - def add_param_context(*keys) - keys.each do |key| - Rack::Honeycomb.add_field(request.env, key, params[key]) - end - end - - def add_context(metadata) - metadata.each do |key, value| - Rack::Honeycomb.add_field(request.env, key, value) - end - end - - def add_user_info_to_honeycomb_event - return unless current_user - - Rack::Honeycomb.add_field(request.env, "user.id", current_user.id) - Rack::Honeycomb.add_field(request.env, "user.username", current_user.username) - end -end diff --git a/spec/services/rss_reader_spec.rb b/spec/services/rss_reader_spec.rb index c6cb6ffae..28611e3b6 100644 --- a/spec/services/rss_reader_spec.rb +++ b/spec/services/rss_reader_spec.rb @@ -6,36 +6,12 @@ vcr_option = { allow_playback_repeats: "true" } -class HoneycombEventStub - attr_reader :data - - def initialize - @data = {} - end - - def add(metadata) - @data.merge!(metadata) - end - - def add_field(key, value) - @data[key] = value - end -end - RSpec.describe RssReader, vcr: vcr_option do let(:link) { "https://medium.com/feed/@vaidehijoshi" } let(:nonmedium_link) { "https://circleci.com/blog/feed.xml" } let(:nonpermanent_link) { "https://medium.com/feed/@macsiri/" } let(:rss_data) { RSS::Parser.parse(HTTParty.get(link).body, false) } - before do - # Honeycomb.init has too many side effects during testing, so we mock the client - event = HoneycombEventStub.new - allow(event).to receive(:send) - client_double = instance_double("Libhoney::TestClient", event: event, events: [event]) - allow(Honeycomb).to receive(:client) { client_double } - end - describe "#get_all_articles" do before do [link, nonmedium_link, nonpermanent_link].each do |feed_url| @@ -123,18 +99,6 @@ RSpec.describe RssReader, vcr: vcr_option do expect(Rails.logger).to have_received(:error).at_least(:once) end - it "logs an article creation error on the observability tool" do - reader = described_class.new - allow(reader).to receive(:make_from_rss_item).and_raise(StandardError) - reader.fetch_user(User.first) - - expected_observable_fields = [ - :user, :feed_url, :item_count, :error, - "error_msg", "trace.trace_id", "trace.parent_id", "trace.span_id" - ] - expect(Honeycomb.client.events.first.data.keys).to eq(expected_observable_fields) - end - it "logs a fetching error on the standard logger" do reader = described_class.new allow(reader).to receive(:fetch_rss).and_raise(StandardError) @@ -142,18 +106,6 @@ RSpec.describe RssReader, vcr: vcr_option do reader.fetch_user(User.first) expect(Rails.logger).to have_received(:error).at_least(:once) end - - it "logs a fetching error on the observability tool" do - reader = described_class.new - allow(reader).to receive(:fetch_rss).and_raise(StandardError) - reader.fetch_user(User.first) - - expected_observable_fields = [ - :user, :feed_url, :item_count, :error, - "error_msg", "trace.trace_id", "trace.parent_id", "trace.span_id" - ] - expect(Honeycomb.client.events.first.data.keys).to eq(expected_observable_fields) - end end describe "#valid_feed_url?" do