Skip to content

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

Merged
dyladan merged 19 commits intoopen-telemetry:mainfrom
aabmass:fix-1774
Aug 11, 2021
Merged

fix(@opentelemetry/exporter-collector): remove fulfilled promises cor…#1775
dyladan merged 19 commits intoopen-telemetry:mainfrom
aabmass:fix-1774

Conversation

@aabmass
Copy link
Copy Markdown
Member

@aabmass aabmass commented Dec 22, 2020 •
edited
Loading

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?

@aabmass aabmass mentioned this pull request Dec 22, 2020
Closed
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 22, 2020 •
edited
Loading

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

Base automatically changed from master to main January 25, 2021 19:26
@aabmass aabmass marked this pull request as ready for review February 23, 2021 21:28
obecny
obecny reviewed Feb 23, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member Author

aabmass commented Feb 24, 2021 •
edited
Loading

@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
Copy link
Copy Markdown
Member

obecny commented Feb 24, 2021

@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
Copy link
Copy Markdown
Member Author

aabmass commented Feb 27, 2021 •
edited
Loading

@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
vmarchaud approved these changes Mar 13, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, could you fix the test ? thanks

@aabmass
Copy link
Copy Markdown
Member Author

aabmass commented Mar 16, 2021

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

@aabmass aabmass requested a review from MSNev as a code owner June 16, 2021 17:41
dyladan
dyladan reviewed Jun 23, 2021
View reviewed changes
Comment thread packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts Outdated
Comment thread packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts Outdated
Comment thread packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts Outdated
Comment thread packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts Outdated
@dyladan
Copy link
Copy Markdown
Member

dyladan commented Jun 30, 2021

Overall LGTM but the tests are still failing

@dyladan dyladan added the enhancement New feature or request label Jun 30, 2021
@dyladan
Copy link
Copy Markdown
Member

dyladan commented Aug 4, 2021

Please fix the tests and the conflict

@aabmass aabmass requested a review from a team August 9, 2021 20:00
@aabmass
Copy link
Copy Markdown
Member Author

aabmass commented Aug 10, 2021 •
edited
Loading

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

@aabmass aabmass requested a review from dyladan August 10, 2021 02:58
rauno56
rauno56 approved these changes Aug 11, 2021
View reviewed changes
Comment thread packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts
obecny
obecny approved these changes Aug 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dyladan dyladan merged commit 90ea0fe into open-telemetry:main Aug 11, 2021
@aabmass aabmass deleted the fix-1774 branch August 11, 2021 15:56
@dyladan dyladan mentioned this pull request Aug 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Flarna Flarna Awaiting requested review from Flarna

@johnbley johnbley Awaiting requested review from johnbley

@legendecas legendecas Awaiting requested review from legendecas

@markwolff markwolff Awaiting requested review from markwolff

@mayurkale22 mayurkale22 Awaiting requested review from mayurkale22

@mwear mwear Awaiting requested review from mwear

@naseemkullah naseemkullah Awaiting requested review from naseemkullah

@OlivierAlbertini OlivierAlbertini Awaiting requested review from OlivierAlbertini

@dyladan dyladan Awaiting requested review from dyladan

4 more reviewers

@obecny obecny obecny approved these changes

@rauno56 rauno56 rauno56 approved these changes

@vmarchaud vmarchaud vmarchaud approved these changes

@MSNev MSNev MSNev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

@opentelemetry/exporter-collector fails to remove fulfilled promises

6 participants

@aabmass @obecny @dyladan @rauno56 @vmarchaud @MSNev