OBPIH-6732 Add Sentry performance and error tracing to React#4943
OBPIH-6732 Add Sentry performance and error tracing to React#4943
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
|
@ewaterman When you get a minute can you rebase this on develop to remove the build failure from read the docs? |
jmiranda
left a comment
There was a problem hiding this comment.
LGTM. Minor improvements and questions. Respond whenever you get a chance.
src/js/config/sentry.jsx
Outdated
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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), | ||
| }), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- clones the dev-ops project
- 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)
- runs the main Ansible script, providing the path to the yml/env files as well as the environment to configure on that host.
- 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
|
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 |
|
@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. |
jmiranda
left a comment
There was a problem hiding this comment.
Looks great. Thanks for changing the env vars.

✨ Description of Change
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)