Skip to content

OBPIH-6732 Add Sentry performance and error tracing to React#4943

Merged
ewaterman merged 4 commits intodevelopfrom
maintenance/OBPIH-6732-sentry-frontend-performance
Jan 23, 2025
Merged

OBPIH-6732 Add Sentry performance and error tracing to React#4943
ewaterman merged 4 commits intodevelopfrom
maintenance/OBPIH-6732-sentry-frontend-performance

Conversation

@ewaterman
Copy link
Member

✨ Description of Change

A concise summary of what is being changed. Please provide enough context for reviewers to be able to understand the change and why it is necessary. If the issue/ticket already provides enough information, you can put "See ticket" as the description.

Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-6732

Description: Parallels the work done in #4898. Start publishing frontend error and performance traces to Sentry.


📷 Screenshots & Recordings (optional)

If this PR contains a UI change, consider adding one or more screenshots here or link to a screen recording to help reviewers visualize the change. Otherwise, you can remove this section.

image

@ewaterman ewaterman self-assigned this Nov 14, 2024
@github-actions github-actions bot added type: maintenance Code improvements, optimizations and refactors, dependency upgrades... domain: frontend Changes or discussions relating to the frontend UI flag: config change Hilights a pull request that contains a change to the app config labels Nov 14, 2024
@codecov
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 7.68%. Comparing base (08b3608) to head (1d41a51).
Report is 193 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff            @@
##             develop   #4943   +/-   ##
=========================================
  Coverage       7.67%   7.68%           
+ Complexity       834     833    -1     
=========================================
  Files            605     610    +5     
  Lines          42451   42521   +70     
  Branches       10308   10315    +7     
=========================================
+ Hits            3260    3268    +8     
- Misses         38711   38773   +62     
  Partials         480     480           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jmiranda
Copy link
Member

@ewaterman When you get a minute can you rebase this on develop to remove the build failure from read the docs?

Copy link
Member

@jmiranda jmiranda left a comment

Choose a reason for hiding this comment

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

LGTM. Minor improvements and questions. Respond whenever you get a chance.

Sentry.httpClientIntegration(), // Adds HTTP request/response body to errors
Sentry.browserTracingIntegration(), // Enables performance tracing
Sentry.replayIntegration({
// Capture all request bodies in the replay except those that contain sensitive info such as passwords.
Copy link
Member

Choose a reason for hiding this comment

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

Ooh, this one is going to be interesting to keep an eye on. Since this is from the perspective of the frontend (so clicking around, posting, etc) we might not need to filter out API requests. But if we are going to see API requests logged here, then we'll want to deal with /api/login as well. And I'm sure there might be other sensitive API and controller actions we'll want to filter out once we do some testing.

Copy link
Member

Choose a reason for hiding this comment

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

Eventually, I would love for all of the following to be configurable if we think that is possible. The reason being if we see we have a vulnerability we should shut the whole thing down. That actually goes for the DSN as well. Is it intuitive enough to remove the DSN var from the environment and restart the server or do we want to control Sentry (and its different capabilitiesL client, tracing, replay) with separate feature flags?

Copy link
Member

Choose a reason for hiding this comment

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

I'm realizing that disabling sentry doesn't necessarily need to require a server restart. We could have an API to fetch the config for sentry. We could either do a generic config fetch in index.js and pass the config object down to the Sentry initialization or let Sentry component handle its own config fetch.

We might want that endpoint to be cached to avoid redundant work on every request (and whenever we change the config on the server side, then an admin user can manually invalidate the cached response for all users).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we can disable it via unsettling the DSN environment variable then restarting the server.

Interesting idea to have the config be modifiable via API. I'm not sure how I feel about passing the whole config around through APIs but it definitely would be nice to have some control over things at runtime. At least a killswitch

new webpack.DefinePlugin({
'process.env.REACT_APP_SENTRY_DSN_FRONTEND': JSON.stringify(process.env.REACT_APP_SENTRY_DSN_FRONTEND),
'process.env.REACT_APP_SENTRY_ENVIRONMENT': JSON.stringify(process.env.REACT_APP_SENTRY_ENVIRONMENT),
}),
Copy link
Member

Choose a reason for hiding this comment

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

As I questioned on Slack, are there other config settings we should be passing in through this approach? For example, is this how we should be defining and passing in the API baseURL and/or the context path? Both of these values should be picked up from the config files (or environment) on the server, but just thinking out loud about whether it makes sense to define all possible config properties here. Or if we did define and default these settings here does that break the current behavior of picking up those values via environment or config files.

Fwiw, it's hard for me to reconcile all of the possible stages and environment for which our config can and should be set.

  • Build (Bamboo build that generates WAR file) -> set defaults, disable everything
  • Deployment (Bamboo build task that deploys WAR file) -> nothing can be done at this point AFAIK
  • Runtime (Tomcat setenv.sh / environment variable) -> override default
  • Runtime (Tomcat setenv.sh -D system properties) -> override default
  • Runtime (openboxes.yml, etc) -> override default (should eventually be refreshed at runtime)

The last three are managed by Ansible. The first two are managed by what's in the source code and any Bamboo build tasks.

Copy link
Member Author

@ewaterman ewaterman Nov 18, 2024

Choose a reason for hiding this comment

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

My idea for the future is to have bamboo build and deploy do nothing except call ansible scripts and to consolidate as much as we can into configuring things via openboxes.yml instead of environment variables.

On the backend I want to have a per-environment yml file (openboxes-obdev3.yml) that lives in the dev-ops project (or better yet a private repo that it can pull from). We can shove all environment-specific settings in those files, then the configure playbook will simply copy that into the host for obdev3. Then eventually we have bamboo running the configure playbook on every deploy. So all we'd need to do to update a setting for an environment is change it in openboxes-xyz.yml then trigger a redeploy and boom it's there.

For the frontend I'm less certain about how that would work. I'd rather not do what I have here as a solution for everything going forward. One possible alternate idea is that we could do something similar to my plan for the backend and create .env.obdev3 files in the dev-ops project (or again a private repo) containing env variables that are specific to each environment, then again have ansible copy the file into the host as a part of the configure playbook. That'd work for the backend as well with any config that MUST be an environment variable so I like it as a long term solution.

https://www.opcito.com/blogs/managing-multiple-environment-configurations-in-react-app

Essentially I want to get to a place where we can merge new environment config to a file somewhere, then simply redeploy and it's there.

Copy link
Member

@jmiranda jmiranda Jan 22, 2025

Choose a reason for hiding this comment

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

We don't need to dive too much deeper into this. I was just curious about whether this was the standard convention. In fact, you don't need to read the following discussion at the moment. We could discuss this on a tech huddle at some point.


To be clear, I'm beyond confused about how we should be going about overriding configuration so just trying to get clarity. But I'm starting to think I need to do a deeper dive into all of this to understand what's possible before I can be involved in discussing what I think is best. I think I'm mostly confused by the fact that we have a bunch of places in which configuration can be defined (application.yml, openboxes.yml, environment variables, system properties) as well as multiple stages at which they can be set (default, test, compile, build, boot, runtime). So I'm going to need to spend a little more time thinking about this, in general.

For example, there are properties (quartz job cron expressions, logging levels) in the application that could be, but most likely won't be runtime configured by most users. There seem to be a few different ways to configure the logging level (i.e. application.yml, logback.yml) but what would happen if we try to override via the command-line.

And then, there are other properties (like database connection settings) that 100% need to be provided to the application through some override mechanism during boot (openboxes.yml, environment variable, system property). But setting those overrides depends on your deployment mechanism.

  • For PIH, we use Bamboo/Ansible to automate writing of openboxes.yml to a VM file system so that it's accessible to the application when it boots up.
  • For some other org they might use AWS Elastic Beanstalk or GCP App Engine, wherein the admin sets up the configuration in a nice little UI and those properties get passed along to the application (presumably through an environment variable).
  • Someone else might use Jenkins and AWS EBS, but instead of manually editing the configuration they ask Jenkins to generate a config file (like you suggested), possibly pulling in some values from the vault and then stuff everything into a file in the WAR. The application discovers these overrides through classloading (classpath:WEB-INF/classes/openboxes.obdev3.yml)
  • Crazy guy over there decides he's going to use Rancher and a remote configuration mechanism (Spring Cloud Config) to configure all of the config properties for the application including the database settings.

Anyway, there are so many different layers to the problem and I don't actually have a good sense of what is a best practice, but it feels like we should be moving toward a scenario where we support environment variables, system properties, and remote config servers. I just don't know yet how to get there.

My idea for the future is to have bamboo build and deploy do nothing except call ansible scripts and to consolidate as much as we can into configuring things via openboxes.yml instead of environment variables.

So that's probably an ok approach for now, but once we start venturing down the path of using Docker and/or Kubernetes (maybe Terraform) we'll probably need to set up an environment in different ways. I have always thought of application.yml as the default, openboxes.yml as an override for environments where it makes sense (manual provisioning and configuration), but once you get into more complicated deployment scenarios, it seems like a best practice to stay away from the file system when providing configuration. Therefore environment variables and system properties should be prioritized.

On the backend I want to have a per-environment yml file (openboxes-obdev3.yml) that lives in the dev-ops project (or better yet a private repo that it can pull from).

Again this isn't an important discussion for this PR, but was just curious ... What is the advantage of having hard-coded files over setting configuration properties via the command-line. As I alluded to above, I'm actually thinking we want to move away from config files altogether because it's a bigger PITA to deal when using Docker and Kubernetes (i.e. you need to mount a volume to store the config file).

For the frontend I'm less certain about how that would work. I'd rather not do what I have here as a solution for everything going forward.

Yeah I think that's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I appreciate all the thoughts on this.

The main advandage of config files IMO is having the config be version controlled. We can see the history of what we changed and why, which is very helpful.

Of course anyone should be able to provide whatever command line overrides that they want, but having that env-specific config live in (or closer to) some other 3rd party app (like Bamboo or Jenkins or whatever) essentially ties the project to that 3rd party app in a way that I want to move away from. I want whatever app is being used to configure hosts (Bamboo for us) to be as dumb as possible. It just:

  1. clones the dev-ops project
  2. either clones a dev-ops-secrets project containing per environment yml/env files (this is what I want us to do) or has those settings configured directly in the 3rd party app (this is the part that is customizable depending on the CD app you use)
  3. runs the main Ansible script, providing the path to the yml/env files as well as the environment to configure on that host.
  4. The scripts run and configure the host.

That way if we for example switch from Bamboo to Jenkins, we don't need to copy a bunch of logic over, just these steps. The rest lives in scripts that we manage.

If people want to clone our devops project so that they can configure their own envs independently, they just need to customize step 2 to fit with whatever setup they have.

Of course if people want to do things differently, we shouldn't make it extremely hard to do so. If they want to provide commandline overrides, we should let them. But as a base point, I think the above will cover most cases (and if they want to do something drastically different, they can write their own CD flow).

In a non-open source project the yml/env files would simply live in the app itself so things would be a little simpler in that we could skip the step of copying the env/yml config in and telling it where to look for it. We'd just specify the environment when building, and internally it'd pull the right file. We're different though in that we don't want to leak PIH env data into the main project so we need to be able to push that config in externally. Fundamentally I don't think that changes much though. It just adds an extra step.

For the frontend, everywhere I look people are saying use .env files for environment specific config, which makes sense to me so that's the direction I was leaning towards but I'll be the first to admit I'm not a frontend expert so I'm not sure what's best.

It can definitely be an ongoing discussion

@ewaterman
Copy link
Member Author

ewaterman commented Jan 10, 2025

As an update (since it's been a while):

@jmiranda I updated the review to address your comments. I think being able to configure things on the live env would be great so that we can do things like disable frontend features without needing to redeploy. I'd like to look into how we can go about that (maybe I can do a 🌟discovery🌟).

I see the current state of configuring the values here via environment variables and webpack as temporary. I want to start working in Ansible soon so that we can use either a runtime JS config, or a .env file to better populate per-environment settings in the frontend. I'd love for this to all be configured automatically on our envs and then we can override it if we want (ex to disable sentry).

The solution here is just to get something functional while I (eventually) work on that

@jmiranda
Copy link
Member

@ewaterman Sorry, this one wasn't showing up in the query "Awaiting review by me" (possibly because I already reviewed it once). I'll get to this next week.

image

Copy link
Member

@jmiranda jmiranda left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for changing the env vars.

@ewaterman ewaterman merged commit d8d1353 into develop Jan 23, 2025
8 checks passed
@ewaterman ewaterman deleted the maintenance/OBPIH-6732-sentry-frontend-performance branch January 23, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: frontend Changes or discussions relating to the frontend UI flag: config change Hilights a pull request that contains a change to the app config type: maintenance Code improvements, optimizations and refactors, dependency upgrades...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants