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

91828: Create general monitor classes #19078

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -2115,8 +2115,8 @@ lib/va_profile/profile/v3/health_benefit_bio_response.rb @department-of-veterans
lib/va_profile/profile/v3/service.rb @department-of-veterans-affairs/vfs-authenticated-experience-backend @department-of-veterans-affairs/vfs-mhv-integration @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/controllers/v0/profile/contacts_controller_spec.rb @department-of-veterans-affairs/vfs-authenticated-experience-backend @department-of-veterans-affairs/vfs-mhv-integration @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
spec/lib/va_profile/profile/v3/service_spec.rb @department-of-veterans-affairs/vfs-authenticated-experience-backend @department-of-veterans-affairs/vfs-mhv-integration @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
lib/logging/third_party_transaction.rb @department-of-veterans-affairs/backend-review-group
spec/lib/logging/third_party_transaction_spec.rb @department-of-veterans-affairs/backend-review-group
lib/logging @department-of-veterans-affairs/backend-review-group @department-of-veterans-affairs/va-api-engineers
spec/lib/logging @department-of-veterans-affairs/backend-review-group @department-of-veterans-affairs/va-api-engineers
spec/lib/zero_silent_failures @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
Brewfile @department-of-veterans-affairs/backend-review-group
docker-compose-deps.yml @department-of-veterans-affairs/backend-review-group
Expand Down
2 changes: 2 additions & 0 deletions lib/burials/monitor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ module Burials
# Monitor functions for Rails logging and StatsD
#
class Monitor < ::ZeroSilentFailures::Monitor
include Logging::Monitor
Copy link
Contributor

Choose a reason for hiding this comment

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

will this cause issues since there would now be 2 constructors? and also a double include/inherit


# statsd key for api
CLAIM_STATS_KEY = 'api.burial_claim'

Expand Down
32 changes: 32 additions & 0 deletions lib/logging/monitor.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# frozen_string_literal: true

module Logging
class Monitor < ::ZeroSilentFailures::Monitor
Copy link
Contributor

Choose a reason for hiding this comment

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

probably should not inherit the ZSF monitor. try and keep this a module/include/mixin with standalone functions
i think this will cause confusion too with 'circular inheritance'

def initialize(service)
@service = service
super(@service)
end

##
# log GET request
#
# @param message [String]
# @param metric [String]
# @param form_type [String]
# @param user_account_uuid [User]
#
def track_request(message, metric, form_type = 'Unknown Form Type', user_account_uuid = nil, call_location: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of form_type have it be 'context' so what ever gets sent will just be appended to the logger payload.
also probably will need to allow 'tags' to be given, but those should be required

function, file, line = parse_caller(call_location)

StatsD.increment(metric)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we leave the metric to whatever the team wants or should we add a bit more structure so they can only pass in the prefix?
i.e. {STATS_KEY}.error

Copy link
Contributor

Choose a reason for hiding this comment

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

For these functions I would let the metric be passed to the function. Also, possibly the 'log level' [info, warn, error] unless you want to split out or specify that it will log as an error in the function name.

Rails.logger.error("#{form_type} #{message}",
{
statsd: metric,
user_account_uuid:,
function:,
file:,
line:
})
end
end
end
99 changes: 99 additions & 0 deletions lib/logging/sidekiq.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
# frozen_string_literal: true

module Logging
class Sidekiq < Monitor
def initialize(service)
@service = service
super(@service)
end

# ##
# log info request
#
# @param message [String]
# @param metric [String]
# @param claim [SavedClaim]
# @param benefits_intake_uuid [FormSubmissions]
# @param user_account_uuid [User]
# @param additional_context [Hash]
#
# rubocop:disable Metrics/ParameterLists
def track_claim_submission(message, metric, claim, benefits_intake_uuid,
user_account_uuid, additional_context, call_location: nil)
function, file, line = parse_caller(call_location)

StatsD.increment(metric)
Rails.logger.info(message.to_s,
{
statsd: metric,
user_account_uuid:,
claim_id: claim&.id,
benefits_intake_uuid: benefits_intake_uuid,
confirmation_number: claim&.confirmation_number,
additional_context:,
function:,
file:,
line:
})
end

# ##
# log warn request
#
# @param message [String]
# @param metric [String]
# @param claim [SavedClaim]
# @param benefits_intake_uuid [FormSubmissions]
# @param user_account_uuid [User]
# @param additional_context [Hash]
#
def track_claim_submission_warn(message, metric, claim, benefits_intake_uuid,
user_account_uuid, additional_context, call_location: nil)
function, file, line = parse_caller(call_location)

StatsD.increment(metric)
Rails.logger.warn(message.to_s,
{
statsd: metric,
user_account_uuid:,
claim_id: claim&.id,
benefits_intake_uuid: benefits_intake_uuid,
confirmation_number: claim&.confirmation_number,
additional_context:,
function:,
file:,
line:
})
end

# ##
# log error request
#
# @param message [String]
# @param metric [String]
# @param claim [SavedClaim]
# @param benefits_intake_uuid [FormSubmissions]
# @param user_account_uuid [User]
# @param additional_context [Hash]
#
def track_claim_submission_error(message, metric, claim, benefits_intake_uuid,
user_account_uuid, additional_context, call_location: nil)
function, file, line = parse_caller(call_location)

StatsD.increment(metric)
Rails.logger.error(message.to_s,
{
statsd: metric,
user_account_uuid:,
claim_id: claim&.id,
benefits_intake_uuid: benefits_intake_uuid,
confirmation_number: claim&.confirmation_number,
additional_context:,
function:,
file:,
line:
})
end
end
# rubocop:enable Metrics/ParameterLists
end
8 changes: 4 additions & 4 deletions lib/zero_silent_failures/monitor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ def log_silent_failure_avoided(additional_context, user_account_uuid = nil, call
})
end

private

attr_reader :service

# parse information from the `caller`
#
# @see https://alextaylor.ca/read/caller-tricks/
Expand All @@ -78,5 +74,9 @@ def parse_caller(call_location)
call_location ||= caller_locations.second # default to location calling 'log_silent_failure...'
[call_location.base_label, call_location.path, call_location.lineno]
end

private

attr_reader :service
Comment on lines +78 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

inherited private functions should be callable from the subclass

end
end
2 changes: 2 additions & 0 deletions modules/pensions/lib/pensions/monitor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ module Pensions
# @todo abstract, split logging for controller and sidekiq
#
class Monitor < ::ZeroSilentFailures::Monitor
include Logging::Monitor

# statsd key for api
CLAIM_STATS_KEY = 'api.pension_claim'

Expand Down
36 changes: 36 additions & 0 deletions spec/lib/logging/monitor_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# frozen_string_literal: true

require 'rails_helper'
require 'zero_silent_failures/monitor'
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed here?

require 'logging/monitor'

RSpec.describe Logging::Monitor do
let(:service) { 'test-application' }
let(:monitor) { described_class.new(service) }
let(:call_location) { double('Location', base_label: 'method_name', path: '/path/to/file.rb', lineno: 42) }
let(:metric) { 'api.monitor.404' }
let(:form_type) { '21P-50EZ' }
let(:user_account_uuid) { '123-test-uuid' }
let(:payload) do
{
statsd: 'OVERRIDE',
user_account_uuid:,
function: call_location.base_label,
file: call_location.path,
line: call_location.lineno
}
end

context 'with a call location provided' do
describe '#track_request' do
it 'logs a request with call location' do
payload[:statsd] = 'api.monitor.404'

expect(StatsD).to receive(:increment).with('api.monitor.404')
expect(Rails.logger).to receive(:error).with('21P-50EZ 404 Not Found!', payload)

monitor.track_request('404 Not Found!', metric, form_type, user_account_uuid, call_location:)
end
end
end
end
91 changes: 91 additions & 0 deletions spec/lib/logging/sidekiq_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# frozen_string_literal: true

require 'rails_helper'
require 'zero_silent_failures/monitor'
require 'logging/monitor'
require 'logging/sidekiq'

RSpec.describe Logging::Sidekiq do
let(:service) { 'test-application' }
let(:monitor) { described_class.new(service) }
let(:call_location) { double('Location', base_label: 'method_name', path: '/path/to/file.rb', lineno: 42) }
let(:metric) { 'api.monitor.sidekiq' }
let(:user_account_uuid) { '123-test-uuid' }
let(:benefits_intake_uuid) { '123-test-uuid' }
let(:additional_context) { { file: 'foobar.pdf', attachments: ['file.txt', 'file2.txt'] } }
let(:claim) do
FactoryBot.create(
:pensions_module_pension_claim,
form: {
veteranFullName: {
first: 'Test',
last: 'User'
},
email: 'foo@foo.com',
veteranDateOfBirth: '1989-12-13',
veteranSocialSecurityNumber: '111223333',
veteranAddress: {
country: 'USA',
state: 'CA',
postalCode: '90210',
street: '123 Main St',
city: 'Anytown'
},
statementOfTruthCertified: true,
statementOfTruthSignature: 'Test User'
}.to_json
)
Comment on lines +17 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

try to avoid using a specific form to test (i recently added a basic generic SaveClaim factory in my pr)
minimally - why not just FactoryBot.build(:pension_module_pension_claim)?

end
let(:payload) do
{
statsd: 'OVERRIDE',
user_account_uuid: user_account_uuid,
claim_id: claim&.id,
benefits_intake_uuid: benefits_intake_uuid,
confirmation_number: claim&.confirmation_number,
additional_context: additional_context,
function: call_location.base_label,
file: call_location.path,
line: call_location.lineno
}
end

context 'with a call location provided' do
describe '#track_claim_submission' do
it 'logs a request with call location' do
payload[:statsd] = 'api.monitor.sidekiq'

expect(StatsD).to receive(:increment).with('api.monitor.sidekiq')
expect(Rails.logger).to receive(:info).with('Lighthouse::Job submission to LH attempted', payload)

monitor.track_claim_submission('Lighthouse::Job submission to LH attempted', metric, claim,
benefits_intake_uuid, user_account_uuid, additional_context, call_location:)
end
end

describe '#track_claim_submission_warn' do
it 'logs a request with call location' do
payload[:statsd] = 'api.monitor.sidekiq'

expect(StatsD).to receive(:increment).with('api.monitor.sidekiq')
expect(Rails.logger).to receive(:warn).with('Lighthouse::Job submission to LH failure', payload)

monitor.track_claim_submission_warn('Lighthouse::Job submission to LH failure', metric, claim,
benefits_intake_uuid, user_account_uuid, additional_context, call_location:)
end
end

describe '#track_claim_submission_error' do
it 'logs a request with call location' do
payload[:statsd] = 'api.monitor.sidekiq'

expect(StatsD).to receive(:increment).with('api.monitor.sidekiq')
expect(Rails.logger).to receive(:error).with('Lighthouse::Job submission to LH exhausted!', payload)

monitor.track_claim_submission_error('Lighthouse::Job submission to LH exhausted!', metric, claim,
benefits_intake_uuid, user_account_uuid,
additional_context, call_location:)
end
end
end
end
Loading