newrelic / gatsby-plugin-newrelic

A Gatsby plugin for instrumenting your site with New Relic's Browser Agent
https://opensource.newrelic.com/projects/newrelic/gatsby-plugin-newrelic
Apache License 2.0
8 stars 19 forks source link

Adjust env config #53

Closed herecydev closed 2 years ago

herecydev commented 2 years ago

This plugins configuration system is needlessly complicated, developers are free to configure environmental options before passing them to the plugin. With the current system, you are forced to set environment variables which might not be readily possible (and varies between unix/windows).

Instead by dropping this concept and accepting a single config this will simplify all scenarios

devfreddy commented 2 years ago

@herecydev Thanks for the suggestion. We started with something similarly simple but quickly needed to support multiple environments. This was done intentionally to try and handle additional logic for users instead of having everyone come up with their own way, while still providing a simple default.

I can see how the "simple default" is not so simple given your callout and because of some of this code - https://github.com/newrelic/gatsby-plugin-newrelic/blob/master/src/gatsby/on-render-body.js#L22-L26

I wonder if we could improve this while keeping backwards compatibility, I'm thinking something like this:

  1. Have a better default that doesn't require you to set that env variable and uses the only (or the first?) config you have setup in configs
  2. Make the env variable approach work better across the spectrum of environments - would love your help with any suggestions here on how to make this work in a Windows environment for example.
  3. Better document both behaviors/approaches

What do you think @herecydev ?

herecydev commented 2 years ago

I originally set out with your first point in mind (if you're interested see my first commit), it's possible to support both but introduces, in my opinion, a bespoke way of solving an already solved problem. As a well documented as a language feature, questions can be answered in many forums (stack overflow, gatsby's github, etc) and the solutions are not tailored to a specific plugin.

So I actually deeply disagree with your point here: "This was done intentionally to try and handle additional logic for users instead of having everyone come up with their own way, while still providing a simple default".

It's not a simple default in that you have to repeat for all environments or set a environment key that's not needed. It's not aligned with other gatsby plugins either; it's not an idiomatic way of solving the problem.

I'm not clear what you mean by point 2. It already does work with windows, if you follow my first commit this would improve the situation without compromising on backwards compat.

devfreddy commented 2 years ago

Apologies, I wasn't saying I succeeded in providing a simple default. It definitely should be augmented for that.

I do think it prudent to provide backwards compat support for what's there now, at least for a time. Is there any kind of Gatsby-ecosystem-specific way to handle deprecations of config settings?

We could support both config and configs for now, with one being marked as deprecated and give preferential treatment to config. That gives a decent path forward for existing and new. How's that feel @herecydev ?

cc: @jpvajda who might have an opinion as well.

jpvajda commented 2 years ago

@herecydev This improvement seems to follow patterns we've used on our other New Relic Gatsby sites. Thanks for taking the time to submit this PR! I'll share this with my engineering team as we are the OSS maintainers for this plugin at the moment. But I'm all for this change!

herecydev commented 2 years ago

I suggest we create a minor release that deprecates the configs, that would address the compatability @devfreddy. Then follow up with removing it in a major release.

If you're happy with that I can reinstate some of that logic and update the readme to indicate the change

devfreddy commented 2 years ago

Sounds great to me @herecydev, appreciate you spending time hashing things out and for making a contribution. 🥂

jpvajda commented 2 years ago

@herecydev I was just following up if you still had interest in contributing this change.

herecydev commented 2 years ago

@jpvajda Definitely, sorry it's just been a busy week. I'll find some time on the weekend for the alterations

jpvajda commented 2 years ago

@herecydev no worries! It's opensource, so we appreciate your contributions. 💯

herecydev commented 2 years ago

I think I've achieved a PR where both systems will be supported. The readme will direct the user to the more idiomatic style and the code will raise a deprecation warning if the older style is used.

jpvajda commented 2 years ago

@herecydev Thanks for doing the work on this PR ! I will review this today to see if we can get it merged in.

roadlittledawn commented 2 years ago

when we merge we may want do a major/minor version bump.

to incorporate that on our sites will require updating our config and update relevant account in NR1 for change in reporting (create/get new applicationID in whatever account)

jpvajda commented 2 years ago

I am going to merge this into main, then merge the next branch into main, and kick off release of the plugin to get a new version up to date on NPM.

nr-opensource-bot commented 2 years ago

:tada: This PR is included in version 2.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

herecydev commented 2 years ago

If you want to ship a pre release I can test that all works correctly

dan-kirkham commented 2 years ago

It works perrfectly, the latest npm tag is still set to 2.2.0 if you want to update that

jpvajda commented 2 years ago

@dan-kirkham Thanks for pointing that out, it looks like this change update bumped the next version of the NPM package, so I can look into updating the latest version.

jpvajda commented 2 years ago

@dan-kirkham I just pushed a latest release.

https://www.npmjs.com/package/gatsby-plugin-newrelic/v/2.2.2