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

Resolve non-Facilities team uses of the VADOTGOV_FacilityLocator lighthouse API consumer key #18351

Closed
2 of 6 tasks
jilladams opened this issue Jun 18, 2024 · 16 comments
Closed
2 of 6 tasks
Assignees
Labels
Blocked Issues that are blocked on factors other than blocking issues. Facilities API Vets-api API that powers Facilities products and the Facility Locator Facilities Facilities products (VAMC, Vet Center, etc) Needs refining Issue status Product management sitewide Technical debt VA.gov frontend CMS team practice area

Comments

@jilladams
Copy link
Contributor

jilladams commented Jun 18, 2024

Problem Statement

VADOTGOV_FacilityLocator is our Lighthouse Facilities API consumer. This user is used in 2 known cases:

  1. facilities-api - which we own and operate (with endpoints like /facilities_api/v2/va)
  2. https://api.va.gov/v1/facilities/va which we do NOT own/operate. wrong. turns out we own this, too.

What: We would like #2 to stop using this API user / key.
Why: Because then we end up owning finding / resolving traffic to deprecating endpoints, as happened with th V0 > V1 cutover

Context

This API originates here: https://github.com/department-of-veterans-affairs/vets-api/tree/master/lib/lighthouse/facilities
The API consumer's key is set here: https://github.com/department-of-veterans-affairs/vets-api/blob/master/lib/lighthouse/facilities/configuration.rb#L19 (lighthouse.facilities.api_key, which belongs to the Facilities team's VADOTGOV_FacilityLocator API consumer)

We don't know who owns this API. We have asked many places:

Contributors include:

  • Philip Becker (4 years ago): former BE Tools lead
  • Lance Sanchez (4 years ago): Still with Lighthouse. Contrib'ed to this API, helping cut it over to use Lighthouse.
  • Michael Pelz Sherman (4ya): Also with Lighthouse, contrib'ed pagination stuff
  • Andrew Herzberg (this month): On VA Mobile API team. Helped create the V1 endpoint.
  • Patrick Vinograd (2020): Did COVID stuff in the API

ACs

  • Figure out who owns this API
  • Figure out if it can be deprecated
  • If not, ask owners to get their own API consumer from Lighthouse, to use in vets-api seems like the wrong direction to go, since we own it.
  • Talk to Medical Copays and see if they are using this legacy API client/library still. If still using it, talk to them about using V1 that Mobile updated.
  • Talk to Mobile about decommissioning and removing the main app API (not V1) library after CovidVaccine is removed.
  • Ask Mobile if they want to take ownership of "Main App V1 library". If they do not want to, we can keep ownership of it and make V3 of facilities API use V2 of Main App in the future.
@jilladams jilladams added VA.gov frontend CMS team practice area Facilities Facilities products (VAMC, Vet Center, etc) Needs refining Issue status Product management Facilities API Vets-api API that powers Facilities products and the Facility Locator Technical debt labels Jun 18, 2024
@jilladams
Copy link
Contributor Author

Convo with prior contributors about what is going on here: https://dsva.slack.com/archives/C0FQSS30V/p1718747443195809

tl;dr: Patrick Vinograd can help us understand, but not right now. We'll come back to this when he has more time later.

@eselkin
Copy link
Contributor

eselkin commented Jun 18, 2024

This delineation seems correct though.

@jilladams jilladams added the Blocked Issues that are blocked on factors other than blocking issues. label Jun 18, 2024
@jilladams
Copy link
Contributor Author

Facilities owns it: https://github.com/department-of-veterans-affairs/platform-atlas/blob/6647cda5fff9fc8a344952f3d5ad88b0c65c6ba7/vets-api-ASSIGNED-CODEOWNERS#L1406

So: we can decide to prune this tech debt at any point (with appropriate comms, scheduling, etc). We also now know that users include VA Mobile app who updated it to V1 LH, bless them.

@jilladams
Copy link
Contributor Author

Discussion with IIR today about /v0/facilities (the legacy API client): https://dsva.slack.com/archives/C0FQSS30V/p1719428540258619. Salient points:

@mmiddaugh I think we should try to chase down whether we should have ownership of this sooner than later if we can, since it's still getting traffic. Let me know if there's some way I can help?

@patrickvinograd
Copy link

patrickvinograd commented Jul 5, 2024

Facility APIs:
Controller: app/controllers/v1/facilities/va_controller.rb
Route: api.va.gov/v1/facilities/va
Calls methods on Lighthouse::Facilities::Client.new aka uses LH v0 API

Controller: modules/facilities_api/app/controllers/facilities_api/v1/va_controller.rb
Route: api.va.gov/facilities_api/v1/va
Instantiates: FacilitiesApi::V1::Lighthouse::Client.new

Controller: modules/facilities_api/app/controllers/facilities_api/v2/va_controller.rb
Route: api.va.gov/facilities_api/v2/va
Instantiates: FacilitiesApi::V2::Lighthouse::Client.new

API traffic:
Metrics Explorer | Datadog (ddog-gov.com)

Other Client Uses:
MedicalCopays::ZeroBalanceStatements -> Lighthouse::Facilities::Client
CovidVaccine::V0::FacilityLookupService -> Lighthouse::Facilities::Client
CovidVaccine::V0::FacilitySuggestionService -> Lighthouse::Facilities::Client

Mobile::FacilitiesHelper -> Lighthouse::Facilities::Client (per feature flag)
Mobile::FacilitiesHelper -> Lighthouse::Facilities::V1::Client (per feature flag)

V0::HealthcareApplicationController -> Lighthouse::Facilities::V1::Client

Non-Uses:
VAOS::V2::FacilitiesController - uses a facility service in the MAP platform, not LH

API Clients:

lib/lighthouse/facilities/client.rb
Lighthouse::Facilities::Client
Invokes: /services/va_facilities/v0/facilities (Lighthouse v0 API)

lib/lighthouse/facilities/v1/client.rb
Lighthouse::Facilities::V1::Client
Invokes: /services/va_facilities/v1/facilities (Lighthouse v1 API)

Both main app clients rely on the same Lighthouse::Facilities::Configuration class
• Same base_path = Settings.lighthouse.facilities.url
• Same apiKey = Settings.lighthouse.facilities.api_key

modules/facilities_api/app/services/facilities_api/v1/lighthouse/client.rb
FacilitiesApi::V1::Lighthouse::Client
Invokes: /services/va_facilities/v0/facilities (Lighthouse v0 API)
Configuration = V1::Lighthouse::Configuration
• base_path = Settings.lighthouse.facilities_url
• api_key = Settings.lighthouse.facilities.api_key

modules/facilities_api/app/services/facilities_api/v2/lighthouse/client.rb
FacilitiesApi::V2::Lighthouse::Client
Invokes: /services/va_facilities/v1/facilities (Lighthouse v1 API)
Configuration = V2::Lighthouse::Configuration
• base_path = Settings.lighthouse.facilities_url
api_key = Settings.lighthouse.facilities.api_key

@patrickvinograd
Copy link

flowchart LR
  LHv0[Lighthouse v0 API\n/services/va_facilities/v0/facilities]
  LHv1[Lighthouse v1 API\n/services/va_facilities/v1/facilities]
  mainAppClient[Lighthouse::Facilities::Client]
  mainAppClientv1[Lighthouse::Facilities::V1::Client]
  moduleClientv1[FacilitiesApi::V1::Lighthouse::Client]
  moduleClientv2[FacilitiesApi::V2::Lighthouse::Client]
  appApiv1(VA.gov API: api.va.gov/v1/facilities/va\nV1::Facilities::VaController)
  moduleApiv1(VA.gov API: api.va.gov/facilities_api/v1/va\nFacilitiesApi::V1::VaController)
  moduleApiv2(VA.gov API: api.va.gov/facilities_api/v2/va\nFacilitiesApi::V2::VaController)
  appConfig[Lighthouse::Facilities::Configuration]
  appConfigAlias[Lighthouse::Facilities::Configuration]
  moduleConfigv1[FacilitiesApi::V1::Lighthouse::Configuration]
  moduleConfigv2[FacilitiesApi::V2::Lighthouse::Configuration]
  classDef sitewide fill:#90EE90
  class appApiv1,moduleApiv1,moduleApiv2 sitewide;

  copays[MedicalCopays::ZeroBalanceStatements]
  covidVax[CovidVaccine::V0::FacilityLookupService\nCovidVaccine::V0::FacilitySuggestionService]
  mobileA[Mobile::FacilitiesHelper\n*Per Feature Flag]
  mobileB[Mobile::FacilitiesHelper\n*Per Feature Flag]
  healthcareApp[V0::HealthcareApplicationController]

  subgraph appcli [main app API client]
    direction TB
    mainAppClient-->appConfig
  end
  subgraph modcliv1 [module API client v1]
    direction TB
    moduleClientv1-->moduleConfigv1;
  end
  subgraph appcliv1 [main app API client v1]
    direction TB
    mainAppClientv1-->appConfigAlias
  end
  subgraph modcliv2 [module API client v2]
    direction TB
    moduleClientv2-->moduleConfigv2;
  end
  appApiv1-->appcli;
  moduleApiv1-->modcliv1;
  moduleApiv2-->modcliv2;
  appcli-->LHv0;
  modcliv1-->LHv0;
  appcliv1-->LHv1;
  modcliv2-->LHv1;
  copays-->appcli;
  covidVax-->appcli;
  healthcareApp-->appcliv1;
  mobileA-->appcli;
  mobileB-->appcliv1;
Loading

@patrickvinograd
Copy link

The above encapsulates all the uses of LH facilities APIs in vets-api.

In terms of API key usage - there are 4 distinct API clients (the yellow-ish boxes) with 3 distinct configuration classes (both of the "main app" clients use the same configuration class. But all of those configuration classes (and hence all the API clients) refer to the same API key setting: Settings.lighthouse.facilities.api_key. So if there's a desire to split things up, the first step would be to provision a new API key, get it configured in the parameter store and create a new entry for it in the settings file, and then reference it from whichever API client/configuration classes is appropriate.

The two APIs and API clients in the facilities_api module seem clearly to be owned by the sitewide/facilities team, to power the VA.gov facility locator, so if the main goal is to isolate "facility locator usage" vs. "all other usage", then those two Configuration classes in the module should be updated to point to a new Settings entry with the facility-locator specific key.

That leaves the uses of the two clients in the main app:

The V1::Facilities::VaController is being presented at (api.va.gov/v1/facilities/va), and in the Datadog graph I posted two comments ago, is receiving more than zero traffic. If that's not from the facility locator, then it would behoove the sitewide/facilities team to figure out where that API traffic is coming from.

Then there are the non-facilities-API uses - the medical copay, covid vaccine, mobile app, and healthcare application. The three that are pointing to LH v0 API should be confirmed whether they are still active - I believe covid vaccine is slated for decomissioning, not sure about medical copays, and the mobile app may have switched their feature flag to use v1. The two currently pointing to LH v1 API (mobile, healthcare application) are legit uses of the LH facilities API so as above, if that client/configuration can be issued a separate API key from the facility locator uses, then things should be more clear. Do they need separate API keys from one another? Unclear.

Finally, who owns the main app API clients? Unfortunately this is a long-standing problem in vets-api. Ownership of apps/APIs is clear, but the code under /lib frequently slips into shared library territory. The facility locator was the first team to need this integration, but once built, it made sense for other teams to use it. The sitewide team could throw up their hands and say "our app doesn't need that anymore, we're not supporting shared libraries for other VFS teams." and retreat to its module.

However, my opinionated take is that this re-use of upstream integrations constitutes one of the main strategic advantages of vets-api and therefore should be encouraged and accounted for. Otherwise every team will build a private integration with Lighthouse, costing us more time and money and reducing consistency of implementation. So my take is that the facilities team should not only build the facility locator that veterans use, it should be the steward of this facilities client API code as a shared benefit to allow all parts of VA.gov to access this data domain. And you should make it clear to your product owners that that is on your plate and requires resources to support.

@jilladams
Copy link
Contributor Author

@FranECross @Agile6MSkinner @mmiddaugh we need to provide Eli some time in Sprint 8 to review these notes from Patrick, and sort out next steps with the 4 of us, probably. @eselkin let's make sure to discuss in planning tomorrow. I've queued the ticket accordingly.

@eselkin
Copy link
Contributor

eselkin commented Jul 9, 2024

Tried to follow the diagram and mark it up...
Screenshot 2024-07-09 at 3 50 01 PM

But basically:

  1. Should decommission and remove not the library but the endpoint for /v1/facilities/va
  2. Should talk to Medical Copays and see if they are using it still. If still using it, talk to them about using V1 that Mobile updated.
  3. Should talk to Mobile about decommissioning and removing the main app API (not V1) library after CovidVaccine is removed.
  4. Despite the original idea of a shared library being good, V1 of Facilities API was not built on it years ago and we modeled V2 on V1. Facilities API also has CCP client/endpoint in addition to VA client/endpoint. Also the library was not versioned at the time when FacilitiesApi V1 was created. We should ask Mobile if they want to take ownership of "Main App V1 library"? If they do not want to, we can keep ownership of it and make V3 of facilities API use V2 of Main App in the future.

@eselkin
Copy link
Contributor

eselkin commented Jul 9, 2024

The use of our API Key is not necessarily the most important thing, because we are nowhere near our rate limit. We get up to like 400/minute and the limit is 10x that.

@eselkin
Copy link
Contributor

eselkin commented Jul 9, 2024

2 Sources of datadog traffic for /v1/facilties/va

  1. CHAMPVA%20Launcher/3 CFNetwork/1496.0.7 Darwin/23.5.0
    and
  2. Referrers like: https://www.va.gov/find-locations/facility/vha_671GS
    1. but if you go to that page it's calling FacilitiesApi V2 so it must be in some wayback machine or something else.

@jilladams
Copy link
Contributor Author

Deprecation of the V1 endpoint is ticketed: #18487

@jilladams
Copy link
Contributor Author

Adding the remainder of Eli's notes to ACs.

@jilladams
Copy link
Contributor Author

Per Donald McCaughey:
Mobile apps don't update at the same rate and must contain longer backward compatibility.
Mobile team would rather migrate to the facilities-api than contemplate owning the Legacy API client.

Don to have someone on his backend team take a look at scope in order to use the facilities-api, rather than the legacy API client. Their team will need to understand if the data is equivalent between the two, in order to determine if migration has a clear path OR what the effort / timing might be.

@jilladams
Copy link
Contributor Author

Sent follow up email to Don to check in on any progress. cced Michael / Michelle.

@jilladams
Copy link
Contributor Author

https://dsva.slack.com/archives/C018V2JCWRJ/p1732212235321749?thread_ts=1732111200.531149&cid=C018V2JCWRJ

After taking a look at everything we'll definitely update from our current LH facilities service to yours. We'll likely pull the update into our next sprint which starts the first week of december.

WOO! So: after they make that change, we will be able to deprecate any additional portions of the legacy API client. I'm ticketing that work separately and closing this epic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Issues that are blocked on factors other than blocking issues. Facilities API Vets-api API that powers Facilities products and the Facility Locator Facilities Facilities products (VAMC, Vet Center, etc) Needs refining Issue status Product management sitewide Technical debt VA.gov frontend CMS team practice area
Projects
None yet
Development

No branches or pull requests

4 participants