DEV: Add X-Discourse-BrowserPageView response header (#33598)

When we are tracking requests in the `Middleware::RequestTracker`, we
have historically added an `X-Discourse-TrackView` response header
for implicitly tracked requests HTML requests, and also explicitly
tracked page view requests when navigating the Ember app.

Within the past couple of years, we introduced the concept of
Browser Page Views (BPVs), which are recorded when requests are made
via AJAX when navigating the app with Ember, and also piggybacked
onto the first MessageBus request on page load. The former is known
as an explicitly tracked request, the latter is a deferred tracked request.

The explicitly tracked browser page views also add the `X-Discourse-TrackView`
header to the response, so it is hard to differentiate which requests are purely
browser page view requests in the logs.

This commit adds a new response header, `X-Discourse-BrowserPageView`,
that acts in a similar way to the existing `X-Discourse-TrackView`
header, but is specifically for requests that are tracked as BPVs (both explicit
and deferred).

lib/middleware/request_tracker.rb

@@ -126,7 +126,7 @@ class Middleware::RequestTracker
 
     # Message-bus requests may include this 'deferred track' header which we use to detect
     # 'real browser' views.
-    if data[:deferred_track] && !data[:is_crawler]
+    if data[:deferred_track_view] && !data[:is_crawler]
       if data[:has_auth_cookie]
         ApplicationRequest.increment!(:page_view_logged_in_browser)
         ApplicationRequest.increment!(:page_view_logged_in_browser_mobile) if data[:is_mobile]
@@ -174,6 +174,9 @@ class Middleware::RequestTracker
     request ||= Rack::Request.new(env)
     helper = Middleware::AnonymousCache::Helper.new(env, request)
 
+    is_html_request = headers["Content-Type"] =~ %r{text/html}
+    is_ajax_request = request.xhr?
+
     # Since ActionDispatch::RemoteIp middleware is run before this middleware,
     # we have access to the normalised remote IP based on ActionDispatch::RemoteIp::GetIp
     #
@@ -182,27 +185,47 @@ class Middleware::RequestTracker
     # end up as 127.0.0.1.
     request_remote_ip = env["action_dispatch.remote_ip"].to_s
 
-    # Value of the discourse-track-view request header, set in `lib/ajax.js`
+    # This Discourse-Track-View request header is set in `lib/ajax.js`,
+    # whenever the user navigates between Ember routes, to indicate a
+    # browser page view.
     env_track_view = env["HTTP_DISCOURSE_TRACK_VIEW"]
-
-    # Was the discourse-track-view request header set to true? Likely
-    # set by our ajax library to indicate a page view.
     explicit_track_view = status == 200 && %w[1 true].include?(env_track_view)
 
-    # An HTML response to a GET request is tracked implicitly
+    # An HTML response to a GET request is tracked implicitly, these do
+    # not count as browser page views but they do count as legacy page views.
     implicit_track_view =
-      status == 200 && !%w[0 false].include?(env_track_view) && request.get? && !request.xhr? &&
-        headers["Content-Type"] =~ %r{text/html}
+      status == 200 && !%w[0 false].include?(env_track_view) && request.get? && !is_ajax_request &&
+        is_html_request
 
-    # This header is sent on a follow-up request after a real browser loads up a page
-    # see `scripts/pageview.js` and `instance-initializers/page-tracking.js`
-    deferred_track_view = %w[1 true].include?(env["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW"])
+    # This Discourse-Deferred-Track-View header is piggybacked on a
+    # follow-up MessageBus request after a real browser loads up a page
+    # to avoid bots influencing browser page views when loading HTML
+    # versions of a page.
+    #
+    # See `scripts/pageview.js` and `instance-initializers/page-tracking.js`
+    env_deferred_track_view = env["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW"]
+    deferred_track_view = %w[1 true].include?(env_deferred_track_view)
 
-    # This is treated separately from deferred tracking in #log_request, this is
-    # why deferred_track_view is not counted here.
+    # This only indicates that we are tracking a page view of some kind, not
+    # using an API key. In #log_request is where we are determining which
+    # of these count as browser page views.
+    #
+    # TL;DR -- Explicit and Deferred page views count as browser page views (BPVs),
+    # explicit and implicit page views count as legacy page views.
+    #
+    # If this is true, then the X-Discourse-TrackView header is included in
+    # the response.
+    #
+    # If the page view is explicit or deferred, then the X-Discourse-BrowserPageView header
+    # is included in the response.
     track_view = !!(explicit_track_view || implicit_track_view)
 
-    # These are set in the same place as the respective track view headers in the client.
+    # Discourse-Deferred-Track-View-Topic-ID is set in the same place as
+    # Discourse-Deferred-Track-View, piggybacked on MessageBus on initial
+    # page load.
+    #
+    # Discourse-Track-View-Topic-ID is set when navigating to a topic on
+    # Ember route change in ajax.js
     topic_id =
       if deferred_track_view
         env["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW_TOPIC_ID"]
@@ -239,7 +262,7 @@ class Middleware::RequestTracker
         end
       end
 
-    h = {
+    request_data = {
       status: status,
       is_crawler: helper.is_crawler?,
       has_auth_cookie: has_auth_cookie,
@@ -253,12 +276,12 @@ class Middleware::RequestTracker
       timing: timing,
       queue_seconds: env["REQUEST_QUEUE_SECONDS"],
       explicit_track_view: explicit_track_view,
-      deferred_track: deferred_track_view,
+      deferred_track_view: deferred_track_view,
       request_remote_ip: request_remote_ip,
     }
 
-    if h[:is_background]
-      h[:background_type] = if is_message_bus
+    if request_data[:is_background]
+      request_data[:background_type] = if is_message_bus
         if request.query_string.include?("dlp=t")
           "message-bus-dlp"
         elsif env["HTTP_DONT_CHUNK"]
@@ -271,17 +294,17 @@ class Middleware::RequestTracker
       end
     end
 
-    if h[:is_crawler]
+    if request_data[:is_crawler]
       user_agent = env["HTTP_USER_AGENT"]
       user_agent = HttpUserAgentEncoder.ensure_utf8(user_agent) if user_agent
-      h[:user_agent] = user_agent
+      request_data[:user_agent] = user_agent
     end
 
     if cache = headers["X-Discourse-Cached"]
-      h[:cache] = cache
+      request_data[:cache] = cache
     end
 
-    h
+    request_data
   end
 
   def log_request_info(env, result, info, request = nil)
@@ -301,6 +324,8 @@ class Middleware::RequestTracker
     if data
       if result && (headers = result[1])
         headers["X-Discourse-TrackView"] = "1" if data[:track_view]
+        headers["X-Discourse-BrowserPageView"] = "1" if data[:explicit_track_view] ||
+          data[:deferred_track_view]
       end
 
       if @@detailed_request_loggers

spec/lib/middleware/request_tracker_spec.rb

@@ -109,6 +109,30 @@ RSpec.describe Middleware::RequestTracker do
       expect(ApplicationRequest.page_view_anon_browser.first.count).to eq(2)
     end
 
+    it "adds the appropriate response header based on explicit tracking (AJAX requests, BPVs)" do
+      middleware = Middleware::RequestTracker.new(lambda { |env| [200, {}, ["OK"]] })
+      status, headers, response = middleware.call(env("HTTP_DISCOURSE_TRACK_VIEW" => "1"))
+      expect(headers["X-Discourse-TrackView"]).to eq("1")
+      expect(headers["X-Discourse-BrowserPageView"]).to eq("1")
+    end
+
+    it "adds the appropriate response header based on implicit tracking (HTML requests)" do
+      middleware =
+        Middleware::RequestTracker.new(
+          lambda { |env| [200, { "Content-Type" => "text/html" }, ["OK"]] },
+        )
+      status, headers, response = middleware.call(env)
+      expect(headers["X-Discourse-TrackView"]).to eq("1")
+      expect(headers["X-Discourse-BrowserPageView"]).to eq(nil)
+    end
+
+    it "adds the appropriate response header based on deferred tracking (MiniProfiler piggyback, BPVs)" do
+      middleware = Middleware::RequestTracker.new(lambda { |env| [200, {}, ["OK"]] })
+      status, headers, response = middleware.call(env("HTTP_DISCOURSE_DEFERRED_TRACK_VIEW" => "1"))
+      expect(headers["X-Discourse-TrackView"]).to eq(nil)
+      expect(headers["X-Discourse-BrowserPageView"]).to eq("1")
+    end
+
     it "can log requests correctly" do
       data =
         Middleware::RequestTracker.get_data(
@@ -173,7 +197,7 @@ RSpec.describe Middleware::RequestTracker do
         )
       Middleware::RequestTracker.log_request(data)
 
-      expect(data[:deferred_track]).to eq(true)
+      expect(data[:deferred_track_view]).to eq(true)
       CachedCounting.flush
 
       expect(ApplicationRequest.page_view_anon_browser.first.count).to eq(1)
@@ -519,7 +543,7 @@ RSpec.describe Middleware::RequestTracker do
 
     after { Rails.logger.stop_broadcasting_to(fake_logger) }
 
-    let :middleware do
+    let(:middleware) do
       app = lambda { |env| [200, {}, ["OK"]] }
 
       Middleware::RequestTracker.new(app)