Analytics: Compact event properties

[aduth]

🛠 Summary of changes

Updates Analytics to avoid tracking nil values for event_properties.

Why?

• They're effectively ignored by CloudWatch, so there's not a meaningful difference between sending nil and not sending anything at all
• May reduce log storage costs, since we're storing less
• Dealing with nil creates a lot of noise in specs, particularly when adding a new optional property to an existing analytics methods (currently dealing with this in Document all analytics logged through sign_in_spec #10966)
• Simplifies methods in AnalyticsEvents which currently call .compact manually, which is usually added as a workaround to the issued described in the point above

Related Slack discussion: https://gsa-tts.slack.com/archives/C0NGESUN5/p1706906370684589

📜 Testing Plan

Verify that the build passes.

Use make watch_events to verify that any event which may log a nil value (either defaulted or explicit from calling an analytics method) does not include the property in the logged output.

[aduth]

For posterity, a terminal command I used to automate the removal of unnecessary nil values from have_logged_event specs:

find spec -type f -name '*.rb' -exec perl -i -p0e 's/have_logged_event\((([^)]|\n)+?),\n\s+(\w+):\ nil,/have_logged_event(\1,/g' {} \;

[aduth]

Some of the spec assertion revisions here are made challenging by differences in how we assert logging:

expect(@analytics).to receive(:track_event).with('...', kwargs)

vs.

expect(@analytics).to have_logged_event('...', kwargs)

The former is outdated in that it doesn't benefit from all of the features of our FakeAnalytics helper class, which now notably includes nil compact-ing.

It might be easier for me to do a separate pass at converting everything over to have_logged_event before continuing with this.

Edit: This has been addressed separately in #11001 and #11010.

[aduth]

The build is passing here now, so it's ready for review.

A couple related notes:

• I'll plan to bring this up in today's engineering huddle meeting to get some final buy-in for the change
• This doesn't deeply compact nil values so certain nil values can still show up in logs, usually in cases where we pass through some vendor result (e.g. proofing results, reCAPTCHA result). I think it could make sense to compact these as well, but I decided to save that for a follow-on effort if there's interest. I could also see the case being made that it's better to leave those "as-is", since we're essentially passing through a vendor result verbatim anyways.

[aduth]

Nevermind, the build showed as green, but I think GitLab hadn't actually run. There's still some more errors to work through.

[aduth]

Most of the spec changes involved simply dropping any nil value assertions that might have existed.

A few were more complicated, particularly with shared examples where sometimes an analytics value is logged and sometimes it's not. In those cases, often I could call .compact within the assertion itself if the value was known in scope. For some, I resorted to using hash_including and omitting the properties which were sometimes nil, if that value wasn't known in scope (e.g. spec/jobs/get_usps_proofing_results_job_spec.rb). Often these used the "anything" matcher. I think this ought to be improved to assert the specific value we expect, but I don't see this as fundamentally worse than matching with anything.

[aduth]

This will need to wait on a few revisions to dependent log metric queries before being able to merge (as discussed during this week's engineering huddle).

[aduth]

This is unblocked now as of DevOps RC v388.