fix(@opentelemetry/exporter-collector): remove fulfilled promises cor…

[aabmass]

Which problem is this PR solving?

Fixes #1774 and fixes the issue in several other places

Short description of the changes

Move the _onFinish() callback code, which pops the resolved promises off the queue, to a separate then() where a reference to promise is available.

One strange thing here is that the popping code will not run at shutdown with Promise.all(this._sendingPromises) because it is not the same promise pushed in the queue. I think this should be ok though?

[codecov]

Codecov Report

Merging #1775 (3ce99e5) into main (4553b29) will decrease coverage by 0.51%.
The diff coverage is 100.00%.

❗ Current head 3ce99e5 differs from pull request most recent head cc42943. Consider uploading reports for the commit cc42943 to get more accurate results

@@            Coverage Diff             @@
##             main    #1775      +/-   ##
==========================================
- Coverage   92.66%   92.14%   -0.52%     
==========================================
  Files         137      120      -17     
  Lines        4973     3998     -975     
  Branches     1047      844     -203     
==========================================
- Hits         4608     3684     -924     
+ Misses        365      314      -51     

Impacted Files	Coverage Δ
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	100.00% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts	87.50% <0.00%> (-6.25%)	⬇️
packages/opentelemetry-sdk-trace-web/src/types.ts
...kages/opentelemetry-exporter-collector/src/util.ts
...ry-context-zone-peer-dep/src/ZoneContextManager.ts
...telemetry-sdk-trace-web/src/StackContextManager.ts
...ation-xml-http-request/src/enums/AttributeNames.ts
.../opentelemetry-exporter-collector/src/transform.ts
...ry-exporter-collector/src/CollectorExporterBase.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
... and 9 more

[obecny]

it looks good, just wondering one thing here, if onSuccess or onError for any reason will raise an error the resolve will never be called and the promise will stay there forever. What if you resolve the promise first and then call the callback so it will never be a problem ?

[aabmass]

@obecny I don't think I've changed that behavior in this PR, but I am happy to modify.

If onSuccess/onError raise an error, then the promise would reject right? Then, would moving the splice to a .finally() work as well?

[obecny]

@obecny I don't think I've changed that behavior in this PR, but I am happy to modify.

If onSuccess/onError raise an error, then the promise would reject right? Then, would moving the splice to a .finally() work as well?

I know you haven't changed that, but just analyzing all edge cases and this is the last thing that might produce such behaviour with not removing the promise probably very rare but still.

With regards to finally I think it will not change anything in whole flow.

[aabmass]

@obecny I changed it to resolve the promise first and then moved the onSuccess/onError calls into separate continuations, lmk how that looks.

I also added a test in 915156f for this

[vmarchaud]

lgtm, could you fix the test ? thanks

[aabmass]

@vmarchaud any idea what the issue is? It only fails on node8, I think it is something node8 specific.

[dyladan]

Overall LGTM but the tests are still failing

[dyladan]

Please fix the tests and the conflict

[aabmass]

Sorry for the slow progress here. I didn't realize Promise.finally() wasn't available in node 8. I've replaced those areas with the two parameter .then(). The other weird test failures that were only happening in node 8 are passing now

[obecny]

lgtm