Skip to content
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

feat: Origin of agent webpack chunks now changeable #659

Merged
merged 27 commits into from
Sep 12, 2023
Merged

Conversation

cwli24
Copy link
Contributor

@cwli24 cwli24 commented Aug 24, 2023

Allow the agent to operate through a proxy. The agent needs to be able to fetch its source code and send analytics to a different domain, and this solves the former. NPM is not affected.

Overview

In support of NR-152264 / Proxy Support, a configuration has been exposed that will change where the agent chunk is loaded from. In context, our default domain js-agent.newrelic.com is on many ad blocker list today which prevent us from collecting data from end users. The use case here is to circumvent that block by using a proxy to fetch agent assets. (The beacon can also be changed so that analytics data flow throw another--or the same--proxy server.)

init: {
  proxy: {
    beacon,
    assets
  }
}

Note that this applies to agent pulled from the CDN (installation methods: copy+paste or APM-injected). Agent installed via NPM should not be affected by this change, as it's irrelevant.

Related Issue(s)

NR-149705

Testing

Existing CDN / NPM agent tests ensure new code does not break default functionality. New e2e has been added that tests the chunk source switching behavior.

UPDATE: this has been merged with #673 .

@github-actions
Copy link

github-actions bot commented Aug 24, 2023

Asset Size Report

Merging this pull request will result in the following asset size changes:

Agent Asset Previous Size New Size Diff
lite loader 28.35 kB / 9.82 kB (gzip) 28.54 kB / 9.89 kB (gzip) 0.66% / 0.75% (gzip)
lite async-chunk 45.4 kB / 15.03 kB (gzip) 45.53 kB / 15.1 kB (gzip) 0.28% / 0.43% (gzip)
pro loader 45.76 kB / 15.31 kB (gzip) 45.95 kB / 15.38 kB (gzip) 0.41% / 0.46% (gzip)
pro async-chunk 166.12 kB / 52.32 kB (gzip) 166.12 kB / 52.32 kB (gzip) 0% / 0% (gzip)
spa loader 52.18 kB / 17.27 kB (gzip) 52.36 kB / 17.35 kB (gzip) 0.36% / 0.46% (gzip)
spa async-chunk 166.12 kB / 52.32 kB (gzip) 166.12 kB / 52.32 kB (gzip) 0% / 0% (gzip)
lite-polyfills loader 98.81 kB / 31.46 kB (gzip) 99.04 kB / 31.55 kB (gzip) 0.23% / 0.29% (gzip)
lite-polyfills async-chunk 57.86 kB / 17.24 kB (gzip) 57.95 kB / 17.32 kB (gzip) 0.16% / 0.45% (gzip)
pro-polyfills loader 118.79 kB / 37.44 kB (gzip) 119.02 kB / 37.53 kB (gzip) 0.19% / 0.26% (gzip)
pro-polyfills async-chunk 98.94 kB / 26.68 kB (gzip) 99.08 kB / 26.77 kB (gzip) 0.14% / 0.33% (gzip)
spa-polyfills loader 126.77 kB / 39.57 kB (gzip) 126.99 kB / 39.67 kB (gzip) 0.18% / 0.25% (gzip)
spa-polyfills async-chunk 113.82 kB / 31.1 kB (gzip) 113.96 kB / 31.16 kB (gzip) 0.12% / 0.21% (gzip)
worker loader 40.46 kB / 13.69 kB (gzip) 40.65 kB / 13.78 kB (gzip) 0.47% / 0.64% (gzip)
worker async-chunk 45.36 kB / 15.37 kB (gzip) 45.51 kB / 15.44 kB (gzip) 0.33% / 0.42% (gzip)

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #659 (6da45ab) into main (2004f5f) will increase coverage by 0.13%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##             main     #659      +/-   ##
==========================================
+ Coverage   86.63%   86.76%   +0.13%     
==========================================
  Files         135      136       +1     
  Lines        5251     5273      +22     
  Branches      720      724       +4     
==========================================
+ Hits         4549     4575      +26     
+ Misses        641      635       -6     
- Partials       61       63       +2     
Flag Coverage Δ
unit-tests 79.05% <70.83%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/features/page_view_event/aggregate/index.js 87.75% <ø> (+1.75%) ⬆️
src/loaders/agent.js 57.89% <ø> (ø)
src/loaders/api/apiAsync.js 36.53% <0.00%> (-6.32%) ⬇️
src/loaders/configure/configure.js 86.95% <85.71%> (+1.24%) ⬆️
src/common/config/state/init.js 92.98% <100.00%> (-0.13%) ⬇️
src/common/constants/runtime.js 97.36% <100.00%> (ø)
src/common/harvest/harvest.js 100.00% <100.00%> (ø)
src/features/ajax/aggregate/index.js 92.45% <100.00%> (+0.14%) ⬆️
src/features/metrics/aggregate/index.js 89.06% <100.00%> (+0.53%) ⬆️
src/loaders/configure/public-path.js 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/common/url/check-url.js Outdated Show resolved Hide resolved
src/common/url/check-url.js Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 31, 2023

Static Badge

Last ran on September 12, 2023 14:12:55 CDT
Checking merge of (029cdfd) into main (2004f5f)

@metal-messiah
Copy link
Member

I think we need to regroup on the config values and implementation of this. We will likely actually need a new property for both values (something like init.proxy.beacon, init.proxy.assets) instead of overwriting beacon/errorBeacon. This will be necessary for compatibility/sanity with customers reverting back to the standard beacon values in the DB (once those have been changed to a custom value, theres no easy way to get the initial value back for the customer).

I also can't help but feel in combination with #673 that we may be over-engineering all these url checks. The combo of those 2 PRs is growing the lite build by more than 5%, for something that should be relatively benign (just adding a property and overwriting the beacon and asset values if set) and unused by the large majority of customers. Validating the url could likely be accomplished by good docs to follow and less by hard-coding checks and parsing all over the agent so we should circle back on this as well.

@cwli24
Copy link
Contributor Author

cwli24 commented Sep 6, 2023

I think we need to regroup on the config values and implementation of this. We will likely actually need a new property for both values (something like init.proxy.beacon, init.proxy.assets) instead of overwriting beacon/errorBeacon. This will be necessary for compatibility/sanity with customers reverting back to the standard beacon values in the DB (once those have been changed to a custom value, theres no easy way to get the initial value back for the customer).

I also can't help but feel in combination with #673 that we may be over-engineering all these url checks. The combo of those 2 PRs is growing the lite build by more than 5%, for something that should be relatively benign (just adding a property and overwriting the beacon and asset values if set) and unused by the large majority of customers. Validating the url could likely be accomplished by good docs to follow and less by hard-coding checks and parsing all over the agent so we should circle back on this as well.

Reworked: validations removed and the input moved to a new object in the init config.

tests/specs/proxy.e2e.js Outdated Show resolved Hide resolved
@cwli24 cwli24 merged commit 739e2dd into main Sep 12, 2023
13 of 14 checks passed
@cwli24 cwli24 deleted the proxy-assets-path branch September 12, 2023 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants