-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Add new relic gem #11256
Add new relic gem #11256
Conversation
c494954
to
4867b13
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.
👌
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.
Great! I would put it in the global group though.
Gemfile
Outdated
@@ -139,6 +139,7 @@ gem "faraday" | |||
gem "private_address_check" | |||
|
|||
group :production, :staging do | |||
gem 'newrelic_rpm' |
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.
Would it hurt to have it in all environments?
I find it increases the chance to catch compatibility bugs early.
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.
It don't think it would. I fixed up the configuration so we don't run the agent by default in Dev. And I realised I forgot to actually test if the configuration was working, which I have now done.
f016de7
to
ce533ef
Compare
I don't actually know how env variable are set up on each instance, maybe someone more knowledgeable can update the testing note ? |
config/newrelic.yml
Outdated
|
||
# To disable the agent regardless of other settings, uncomment the following: | ||
|
||
# agent_enabled: false |
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 think we probably want to disable the agent by default if there's no API key present (in development for example). I think this file allows interpolation with erb.
Maybe here we can do something like:
agent_enabled: <%= ENV["NEW_RELIC_LICENSE_KEY"].present? %>
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 idea.
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.
the new relic config doesn't allow interpolation any more. I couldn't find a way to disable the agent if the license key is not present, but I updated the config to disable the agent in dev, test and staging.
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.
That's a problem. Not every production instance will have a new relic key. Do we set it to false everywhere and then use environment variables to control it instead?
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.
Yes I think that would be a better solution. Do we need to update any documentation around 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.
Do we need to update any documentation around this ?
Yes.
- The ofn-install repository has a template for the secrets which needs entries with comments.
- The wiki describes optional services, how to set them up etc. It needs to be mentioned there.
8441421
to
2bfe217
Compare
config/newrelic.yml
Outdated
staging: | ||
<<: *default_settings | ||
app_name: 'Open Food Network (Staging)' | ||
agent_enabled: false |
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.
How are we going to test changes before they go to production? It would be great to have this on staging as well. And testing locally?
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.
you can override the configuration by setting the env variable NEW_RELIC_AGENT_ENABLED to true
5c55908
to
f4b9a6f
Compare
# Your application name. Renaming here affects where data displays in New | ||
# Relic. For more details, see https://docs.newrelic.com/docs/apm/new-relic-apm/maintenance/renaming-applications | ||
# Use NEW_RELIC_APP_NAME env variable to customize the application name | ||
app_name: "Open Food Network" |
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 have one question:
Will all the instances share the same key? and NEW_RELIC_APP_NAME will variate depending on the instance location. for example: "Open Food Network (AU)", "Open Food Network (UK)" ?
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.
That's a good question. In the past we would have set up separate keys but if New Relic supports it then we should use the same key, I think. That we we have all the information in one place.
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.
My understanding is we should be able to use the same license key for all the instances, and set up a different name for each instance so we can differentiate the data. See below:
There is the possibility to create extra license key so we could potentially have a different license key for each instance, all reporting to the same New Relic account. Or have a different user with it's own license key for each instance, but that's probably not needed. It's recommended to create extra license for security reason (key rotation) .
More info here: https://docs.newrelic.com/docs/apis/intro-apis/new-relic-api-keys/
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.
Okay, I think that we should roll out with the same key and we can rotate later.
f4b9a6f
to
e2e975d
Compare
I tested this on |
It can be enable as needed by setting NEW_RELIC_AGENT_ENABLED env variable to true and setting a license key
e2e975d
to
0103942
Compare
What? Why?
Related to #10997
It uses the following env varirable to enable new relic agent, setup the licence key and customise application name:
The agent is disable by default so we can enable new relic for the production server that needs it.
What should we test?
Release notes
Changelog Category: Technical changes
The title of the pull request will be included in the release notes.