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

fix(exporter-collector): all http export requests should share same a… #1863

Merged
merged 6 commits into from
Jan 25, 2021
Merged

fix(exporter-collector): all http export requests should share same a… #1863

merged 6 commits into from
Jan 25, 2021

Conversation

blumamir
Copy link
Member

Which problem is this PR solving?

Short description of the changes

Create a single http/https agent in exporter constructor and set the same one in the options for each HTTP request.
As I wrote in the issue, In my opinion it's better to pass an initialized agent to the collector instead of the collector creating one for itself. Since there was no discussion about it, I implemented the fix to stay as close as possible to the current implementation:

  • default is to create an agent for the exporter.
  • to disable agent, user need to pass { keepAlive: false } to the exporter options.
  • Additional agent parameters can be passed in the httpAgentOptions option.
  • In case httpAgentOptions is used, if user set keepAlive: false, a warning will be printed to log and agent will not be used (node's agent allow this configuration, why do we prevent it in the exporter?)

I added a unittest for the change, and tested it locally in a real application, and observed the latency of a single export after warmup. My test application is located in Israel and export spans to collector in aws Ireland. Export time dropped from ~500ms prior to the fix, to ~100ms with the fix.

@codecov
Copy link

codecov bot commented Jan 24, 2021

Codecov Report

Merging #1863 (f57a562) into master (a78bb80) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1863      +/-   ##
==========================================
- Coverage   92.68%   92.66%   -0.02%     
==========================================
  Files         174      174              
  Lines        6040     6040              
  Branches     1284     1284              
==========================================
- Hits         5598     5597       -1     
- Misses        442      443       +1     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@blumamir
Copy link
Member Author

The lint fail is not related to my PR:

lerna ERR! npm run docs-test stderr:
🏊‍♂️ crawling docs/out
[404] https://open-telemetry.github.io/opentelemetry-js/classes/traceapi.html
[404] https://open-telemetry.github.io/opentelemetry-js/classes/propagationapi.html
[404] https://open-telemetry.github.io/opentelemetry-js/classes/contextapi.html
docs/out
  [404] https://open-telemetry.github.io/opentelemetry-js/classes/traceapi.html
  [404] https://open-telemetry.github.io/opentelemetry-js/classes/propagationapi.html
  [404] https://open-telemetry.github.io/opentelemetry-js/classes/contextapi.html
ERROR: Detected 3 broken links. Scanned 55 links in 0.48 seconds.

@naseemkullah naseemkullah merged commit 31ab012 into open-telemetry:master Jan 25, 2021
@obecny
Copy link
Member

obecny commented Jan 25, 2021

@naseemkullah
Copy link
Member

Oops, sorry @obecny, another maintainer approval was required.

@obecny
Copy link
Member

obecny commented Jan 25, 2021

Oops, sorry @obecny, another maintainer approval was required.

it is also a breaking change so I would expect 24 hours wait so more people can have a chance to look at.

@blumamir
Copy link
Member Author

Oops, sorry @obecny, another maintainer approval was required.

it is also a breaking change so I would expect 24 hours wait so more people can have a chance to look at.

why is it a breaking change?

@obecny
Copy link
Member

obecny commented Jan 25, 2021

why is it a breaking change?

You have removed public keepAlive and httpAgentOptions from class

@dyladan dyladan added the bug Something isn't working label Jan 25, 2021
@dyladan
Copy link
Member

dyladan commented Jan 25, 2021

@naseemkullah please when merging ensure one of the changelog labels from lerna.json is applied

@naseemkullah
Copy link
Member

@naseemkullah please when merging ensure one of the changelog labels from lerna.json is applied

Thanks @dyladan, will do!

@dyladan
Copy link
Member

dyladan commented Jan 25, 2021

Additionally, merge requirements were not met for this PR. Merge by a non-maintainer requires at least 2 maintainer approvals. This PR only has 1. I am not going to revert but please keep this in mind in the future.

@naseemkullah
Copy link
Member

@dyladan yes true, this was pointed out by @obecny and acked: #1863 (comment) :)

@dyladan
Copy link
Member

dyladan commented Jan 25, 2021

@blumamir since this is a breaking change can you please document the break in the migration section of the README in another PR?

@blumamir
Copy link
Member Author

@blumamir since this is a breaking change can you please document the break in the migration section of the README in another PR?

sure, I'll send this PR tomorrow.
do you mean to add something like the Upgrade guidelines currently in the README.md?
Should I add 0.15.0 to 0.16.0?

@obecny
Copy link
Member

obecny commented Jan 25, 2021

I guess it will be enough to mention what has been removed.

@blumamir
Copy link
Member Author

@blumamir since this is a breaking change can you please document the break in the migration section of the README in another PR?

#1867

pichlermarc added a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
The beforeEach() hook was not awaited, so afterEach() could run before
it completed, resulting in a client.disconnect() that rejects, and a
mocha hook that calls done() twice.

Refs: open-telemetry#1860

Co-authored-by: Marc Pichler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
7 participants