fix(rum): use single instance of apm across all packages

[vigneshshanmugam]

• fixes bundle should not replace elasticApm if its present on window  #791
• Ensures the following when the agent script is loaded twice or agent script is used along with other framework integrations
  • apm instance is singleton
  • does not patch the window.elasticApm multiple times
  • does not patch XHR, Fetch, History multiple times.

[apmmachine]

💚 Build Succeeded

Expand to view the summary

Build stats

• Build Cause: [Pull request #796 updated]

• Start Time: 2020-05-27T12:45:15.396+0000

• Duration: 92 min 20 sec

Test stats 🧪

Test	Results
Failed	0
Passed	932
Skipped	10
Total	942

Steps errors

Expand to view the steps failures

• Name: Bundlesize

  • Description: #!/bin/bash set -o pipefail npm run bundlesize|tee bundlesize.txt

  • Duration: 1 min 31 sec

  • Start Time: 2020-05-27T12:52:41.934+0000

  • log

• Name: Start Elastic Stack 7.7.0 - @elastic/apm-rum - none

  • Description:

  • Duration: 4 min 2 sec

  • Start Time: 2020-05-27T13:01:16.757+0000

  • log

• Name: Error signal

  • Description: The test failed

  • Duration: 0 min 0 sec

  • Start Time: 2020-05-27T13:40:05.141+0000

  • log

[codecov-commenter]

Codecov Report

Merging #796 into master will increase coverage by 0.52%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
+ Coverage   92.13%   92.66%   +0.52%     
==========================================
  Files          50       50              
  Lines        2289     2290       +1     
  Branches      457      456       -1     
==========================================
+ Hits         2109     2122      +13     
+ Misses        177      165      -12     
  Partials        3        3              

Impacted Files	Coverage Δ
packages/rum-core/src/index.js	100.00% <ø> (ø)
packages/rum/src/apm-base.js	99.05% <ø> (ø)
packages/rum-core/src/bootstrap.js	66.66% <100.00%> (+51.28%)	⬆️
packages/rum-core/src/common/polyfills.js	100.00% <100.00%> (+33.33%)	⬆️
packages/rum-core/src/common/url.js	98.61% <100.00%> (ø)
packages/rum-core/src/common/utils.js	94.90% <100.00%> (+1.31%)	⬆️
packages/rum/src/index.js	100.00% <100.00%> (ø)

[vigneshshanmugam]

@jahtalab Can i get your review before I do the rebase, that wont have a huge change as it would be simply point to rum-core.

[hmdhk]

@vigneshshanmugam , I think the approach taken here is fine but IMO, it doesn't address the core issue which is the fact that the agent is loaded in the page twice, we need to fix that! One idea is to stop having dependency from the framework specific packages to the base package and instead use window.elasticApm

[vigneshshanmugam]

agent is loaded in the page twice

This is a common problem and we have to deal with it as you said. There are couple of options.

1. Exposing apmBase, apm from @elastic/apm-rum-*(react/angular/vue) packages

This would let users to use to initialise the agent from the imports of a single package and would remove the need to import @elastic/apm-rum for initialisation purpose alone. This is already done in VuePlugin and AngularService but react package doesn't expose it.

2. Relying on window.elasticApm in framework integrations

This has huge disadvantages.

• We expect the users to load apmbase before the framework package which would result in window.elasticApm not being found if any of the modules was downloaded async and we cannot dynamically import if the package isn't found.

Looking at both options, I would say No.1 is an easy solution and best approach. I have created an issue stating the above reasons #799.

In any case, this PR is not related to how exports are used. Instead it fixes the issue if the user imported our packages by mistake multiple times and our library has to deal with it.

[hmdhk]

Thanks @vigneshshanmugam for opening the issue, we should address the core issue to prevent this. But we can merge this one as a quick fix.

[hmdhk]

nit: Is there any reason for having a function here instead of just having on the top level?
It seems to me the logic would be simpler if we remove this function.

[vigneshshanmugam]

Just a personal preference. It looked a bit cleaner than having to reassign apmBase like this

let apmBase

if (isBrowser && window.elasticApm) {
  apmBase = window.elasticApm
} else {
  const enabled = bootstrap()
  const serviceFactory = createServiceFactory()
  apmBase = new ApmBase(serviceFactory, !enabled)

  if (isBrowser) {
    window.elasticApm = apmBase
  }
}
export {apmBase}