-
Notifications
You must be signed in to change notification settings - Fork 29
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
send handled/unhandled errors to newrelic #154
Conversation
config/webpack.dev.config.js
Outdated
@@ -4,6 +4,7 @@ const Merge = require('webpack-merge'); | |||
const path = require('path'); | |||
const webpack = require('webpack'); |
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.
Note to Reviewers: Changes in this file are for local testing only. Will be removed before merge.
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.
In the future, if you have changes in a file that are for local testing only, just stash those changes prior to committing to not add confusion to the PR.
f30a3a5
to
b75e002
Compare
@@ -65,7 +65,7 @@ | |||
"clean-webpack-plugin": "^0.1.19", | |||
"codecov": "^3.0.0", | |||
"css-loader": "^0.28.9", | |||
"enzyme": "^3.3.0", | |||
"enzyme": "^3.6.0", |
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.
Note to Reviewers: This is needed to test the ErrorBoundary. Please see enzymejs/enzyme#1255
b75e002
to
41d4347
Compare
41d4347
to
7ae007c
Compare
config/webpack.dev.config.js
Outdated
@@ -4,6 +4,7 @@ const Merge = require('webpack-merge'); | |||
const path = require('path'); | |||
const webpack = require('webpack'); |
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.
In the future, if you have changes in a file that are for local testing only, just stash those changes prior to committing to not add confusion to the PR.
config/webpack.dev.config.js
Outdated
@@ -109,14 +110,19 @@ module.exports = Merge.smart(commonConfig, { | |||
CSRF_TOKEN_API_PATH: '/csrf/api/v1/token', | |||
REFRESH_ACCESS_TOKEN_ENDPOINT: 'http://localhost:18000/login', | |||
DATA_API_BASE_URL: 'http://localhost:8000', | |||
ECOMMERCE_API_BASE_URL: null, | |||
ECOMMERCE_API_BASE_URL: 'http://localhost:18130/api/v2', |
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.
Just FYI, I merged a PR (#156) up that adds this URL and updates the URLs in EcommerceApiService.js
to add /api/v2
, following the convention we've set up with our other API calls.
src/Logging/index.js
Outdated
@@ -0,0 +1,29 @@ | |||
function logInfo(message) { | |||
if (typeof newrelic !== 'undefined') { | |||
newrelic.addPageAction('INFO', { message }); // eslint-disable-line no-undef |
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 disabling this eslint rule each time you access newrelic
, we could add newrelic
as a global in the .eslintrc
file? Example (see docs):
{
"globals": {
"newrelic": false
}
}
src/Logging/index.js
Outdated
|
||
function logAPIErrorResponse(error) { | ||
let { message } = error; | ||
if (error.response) { |
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.
I'm curious where you're getting these variations of errors. Do you have a link to some documentation? It seems mostly inline with axios's error handling documentation: https://github.com/axios/axios#handling-errors.
@@ -5,6 +5,7 @@ import { | |||
CLEAR_DASHBOARD_ANALYTICS, | |||
} from '../constants/dashboardAnalytics'; | |||
import EnterpriseDataApiService from '../services/EnterpriseDataApiService'; | |||
import * as logging from '../../Logging'; |
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.
I would recommend only importing the functions we need in this file, instead of using the wildcard asterisk.
import * as logging from '../../Logging'; | ||
|
||
class ErrorBoundary extends React.Component { | ||
componentDidCatch(error, info) { // eslint-disable-line no-unused-vars |
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.
If you don't need the info
variable, you don't need to provide it as an argument.
@@ -9,6 +9,8 @@ import LoadingMessage from '../LoadingMessage'; | |||
|
|||
import LmsApiService from '../../data/services/LmsApiService'; | |||
|
|||
import * as logging from '../../Logging'; |
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.
Same as above.
src/data/actions/coupons.js
Outdated
@@ -4,6 +4,7 @@ import { | |||
COUPONS_FAILURE, | |||
CLEAR_COUPONS, | |||
} from '../constants/coupons'; | |||
import * as logging from '../../Logging'; |
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.
Same as above.
src/data/actions/csv.js
Outdated
@@ -7,6 +7,7 @@ import { | |||
CLEAR_CSV, | |||
} from '../constants/csv'; | |||
import store from '../store'; | |||
import * as logging from '../../Logging'; |
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.
Same as above.
src/Logging/index.js
Outdated
@@ -0,0 +1,29 @@ | |||
function logInfo(message) { |
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.
I'm wondering if src/Logging/index.js
is the right place for this file. It might make more sense to have as another file in our services
directory instead, e.g. src/data/services/NewRelicService.js
. Thoughts?
7ae007c
to
ac0678a
Compare
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.
LGTM
ac0678a
to
95af224
Compare
ENT-1290