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

12423 remove all statsd from initializers #12426

Merged
merged 0 commits into from
Apr 20, 2023

Conversation

bosawt
Copy link
Contributor

@bosawt bosawt commented Apr 17, 2023

Summary

  • This PR removes unnecessary StatsD reset counters from all initializers. This should help anyone seeing metrics flipping back and forth between count and rate (making metrics appear to be performing unexpectedly)

Related issue(s)

Testing done

  • I started a Rails server and performed various actions to confirm that things worked fine and that StatsD were still reporting as expected

What areas of the site does it impact?

Metric

Acceptance criteria

  • Confirm that your own StatsD metrics still log as expected in `log/statsd.log

@bosawt bosawt requested review from a team as code owners April 17, 2023 21:13
@bosawt bosawt force-pushed the 12423_remove_all_statsd_from_initializers branch from b0e17b7 to 2f43496 Compare April 17, 2023 21:15
@va-vsp-bot va-vsp-bot requested a deployment to master/main/12423_remove_all_statsd_from_initializers April 17, 2023 21:16 In progress
@va-vfs-bot va-vfs-bot temporarily deployed to 12423_remove_all_statsd_from_initializers/main/main April 17, 2023 21:21 Inactive
@bosawt bosawt force-pushed the 12423_remove_all_statsd_from_initializers branch from 2f43496 to 65a91be Compare April 17, 2023 21:30
@github-actions github-actions bot added console-services-review mobile VAOS Va Online Scheduling Contract labels Apr 17, 2023
@va-vsp-bot va-vsp-bot requested a deployment to master/main/12423_remove_all_statsd_from_initializers April 17, 2023 21:30 In progress
@github-actions
Copy link

github-actions bot commented Apr 17, 2023

1 Warning
⚠️ This PR changes 366 LoC (not counting whitespace/newlines).

In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding
200. Expect some delays getting reviews.

File Summary

Files

  • app/mailers/application_mailer.rb (+2/-0)

  • app/workers/evss/disability_compensation_form/metrics.rb (+2/-0)

  • config/initializers/statsd.rb (+0/-178)

  • lib/evss/reference_data/response_strategy.rb (+2/-0)

  • modules/appeals_api/app/workers/appeals_api/add_icn_updater.rb (+1/-0)

  • modules/appeals_api/app/workers/appeals_api/daily_error_report.rb (+1/-0)

  • modules/appeals_api/app/workers/appeals_api/decision_review_report_daily.rb (+1/-0)

  • modules/appeals_api/app/workers/appeals_api/decision_review_report_weekly.rb (+1/-0)

  • modules/appeals_api/app/workers/appeals_api/monthly_stats_report.rb (+2/-0)

  • modules/appeals_api/app/workers/appeals_api/weekly_error_report.rb (+1/-0)

  • modules/check_in/config/initializers/statsd.rb (+2/-0)

  • modules/claims_api/app/workers/claims_api/claim_establisher.rb (+1/-0)

  • modules/claims_api/spec/requests/v1/power_of_attorney_request_spec.rb (+1/-0)

  • modules/covid_vaccine/config/initializers/statsd.rb (+0/-25)

  • modules/covid_vaccine/spec/request/covid_vaccine/v0/facilities_request_spec.rb (+1/-0)

  • modules/mobile/config/initializers/statsd.rb (+1/-76)

  • modules/vaos/config/initializers/statsd.rb (+0/-22)

  • modules/vaos/config/initializers/statsd_instrument_monkeypatch.rb (+0/-45)

  • spec/lib/sidekiq/form526_job_status_tracker/job_tracker_spec.rb (+1/-0)

    Note: We exclude files matching the following when considering PR size:

    *.csv, *.json, *.tsv, *.txt, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/
    

Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

@va-vfs-bot va-vfs-bot temporarily deployed to 12423_remove_all_statsd_from_initializers/main/main April 17, 2023 21:31 Inactive
Copy link
Contributor

@kpethtel kpethtel left a comment

Choose a reason for hiding this comment

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

I don't know much about statsd, but it looks like you're saying we're misusing the increment method. This looks like it should include some work on each team's part to maintain the metrics you're removing. Do we need any followup actions?

rmtolmach
rmtolmach previously approved these changes Apr 18, 2023
Copy link
Contributor

@rmtolmach rmtolmach left a comment

Choose a reason for hiding this comment

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

Looks good. Follow-up with @kpethtel required.

@rmtolmach
Copy link
Contributor

oh wait... I swear all the checks were passing a minute ago. But now they're not. Can you look into those @bosawt?

@bosawt
Copy link
Contributor Author

bosawt commented Apr 18, 2023

I don't know much about statsd, but it looks like you're saying we're misusing the increment method. This looks like it should include some work on each team's part to maintain the metrics you're removing. Do we need any followup actions?

I wouldn't go so far as to say we were 'misusing' it, just that there appears to have been a historic reason we were initializing these to 0, but as far as I can tell, that historic reason is no longer true.

Also, doing an increment reset in the initializers was at worst a no-op until the recent EKS switch, it was causing a ton of metric issues for us. And it's possible we could change how EKS reports metrics to fix it, but for now what was happening was:

  • EKS instance would increment metric X whenever route on EKS was run, for whatever reason, Datadog would classify this metric as a rate type metric
  • Any time BRD instance was deployed, it would 'reset' metric X with that 'increment 0' in the initializer, and for whatever reason, Datadog would classify the metric as count type metric
  • The difference in rate versus count type metrics was taking a metric that was like 'X was incremented 100 times', and making it something really messed up like 'X was incremented 15.43 times'

@bosawt bosawt force-pushed the 12423_remove_all_statsd_from_initializers branch from 65a91be to b35b55d Compare April 18, 2023 20:34
@va-vsp-bot va-vsp-bot requested a deployment to 12423_remove_all_statsd_from_initializers/main/main April 18, 2023 20:35 In progress
@va-vfs-bot va-vfs-bot temporarily deployed to 12423_remove_all_statsd_from_initializers/main/main April 18, 2023 20:40 Inactive
@bosawt bosawt force-pushed the 12423_remove_all_statsd_from_initializers branch from b35b55d to 471c321 Compare April 18, 2023 20:56
@bosawt bosawt requested a review from a team as a code owner April 18, 2023 20:56
@va-vsp-bot va-vsp-bot requested a deployment to 12423_remove_all_statsd_from_initializers/main/main April 18, 2023 20:57 In progress
@bosawt bosawt force-pushed the 12423_remove_all_statsd_from_initializers branch from 471c321 to b828775 Compare April 18, 2023 22:32
@va-vsp-bot va-vsp-bot requested a deployment to 12423_remove_all_statsd_from_initializers/main/main April 18, 2023 22:33 In progress
@bosawt bosawt requested review from kpethtel and rmtolmach April 18, 2023 22:54
@bosawt
Copy link
Contributor Author

bosawt commented Apr 18, 2023

@rmtolmach Finally made it so specs are no longer breaking

Copy link
Contributor

@kpethtel kpethtel left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up. I'm going to file a ticket for my team to review our use of statsd and evaluate whether we need to re-add any of the removed metrics (but without issues).

@bosawt bosawt merged commit b815b4e into master Apr 20, 2023
@bosawt bosawt deleted the 12423_remove_all_statsd_from_initializers branch April 20, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console-services-review mobile VAOS Va Online Scheduling Contract
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants