-
-
Notifications
You must be signed in to change notification settings - Fork 499
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
Move to Newrelic for logging and APM #3710
Conversation
@@ -16,6 +16,7 @@ class ApplicationController < ActionController::Base | |||
|
|||
def current_organization | |||
return @current_organization if @current_organization | |||
return nil unless current_role |
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.
This looks good and correct but is unrelated, yes?
config.lograge.enabled = true | ||
config.lograge.custom_payload do |controller| | ||
{ | ||
host: controller.request.host, | ||
user_id: controller.current_user.try(:id) | ||
user_id: controller.current_user.try(:id), |
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.
oh, maybe the nil
returns were to help these!
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.
Yep, that was why 😁
config/newrelic.yml
Outdated
|
||
common: &default_settings | ||
# Required license key associated with your New Relic account. | ||
license_key: 9942120fb257ae099f776017cd6926a760e6NRAL |
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.
Should this be pulled from an ENV? I think the worst case is someone submitting bogus data to NR, and since we have a free account even if our resources get used up it wouldn't be the worst thing ever, so maybe fine for now.
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.
Good catch. Updated this.
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.
👍 (gotta fix conflict)
Resolves #3706
Description
Adds Newrelic APM integration and removes Skylight.
Once this is merged, I will add info to the Gitbook related to this.
Type of change
Infrastructure change
How Has This Been Tested?
Deployed to staging.