-
Notifications
You must be signed in to change notification settings - Fork 62
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
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
def track_request(message, metric, form_type = 'Unknown Form Type', user_account_uuid = nil, call_location: nil) | ||
function, file, line = parse_caller(call_location) | ||
|
||
StatsD.increment(metric) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Generated by 🚫 Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a confusing overlap with the ZSF monitor. Arguments for and against this logging module being agnostic versus inheriting ZSF.
I thought this library was going to live under lib/common
?
Your thoughts/opinion on making a subclass to controller and sidekiq for saved_claim (since there are things which are not saved_claim which could potentially want to use this library)?
@@ -7,6 +7,8 @@ module Burials | |||
# Monitor functions for Rails logging and StatsD | |||
# | |||
class Monitor < ::ZeroSilentFailures::Monitor | |||
include Logging::Monitor |
There was a problem hiding this comment.
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
# frozen_string_literal: true | ||
|
||
module Logging | ||
class Monitor < ::ZeroSilentFailures::Monitor |
There was a problem hiding this comment.
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 track_request(message, metric, form_type = 'Unknown Form Type', user_account_uuid = nil, call_location: nil) | ||
function, file, line = parse_caller(call_location) | ||
|
||
StatsD.increment(metric) |
There was a problem hiding this comment.
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.
# @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) |
There was a problem hiding this comment.
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
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 | ||
) |
There was a problem hiding this comment.
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)
?
# frozen_string_literal: true | ||
|
||
require 'rails_helper' | ||
require 'zero_silent_failures/monitor' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed here?
private | ||
|
||
attr_reader :service |
There was a problem hiding this comment.
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
Create a general monitor class for vets-api for overall general tracking and sidekiq jobs with claims involved.
Summary
Logging::Monitor
class to hold simple request trackingLogging::Monitor::Sidekiq
for general Claim-based Sidekiq jobsinclude
general classes to Pensions and BurialsRelated issue(s)
Testing done
What areas of the site does it impact?
N/a just new logging classes