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(exporters)!: use transport interface in node.js exporters #4743

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented May 31, 2024

Which problem is this PR solving?

Exporters are very hard to test as everything has to be integration-tested at the moment (see #4116).

This PR does the following for the Node.js exporters:

  • moves the ExporterTransport interface previuously only used by the grpc exporters (introduced in fix(exporter-*-otlp-grpc)!: lazy load gRPC #4432) to the exporter base package
  • untangles the HTTP transport and retry logic by making them both implement the interface
  • makes the HTTP transport and retry logic individually testable
  • removes the header property to reduce the API surface of the OTLPExporterNodeBase and its child classes
    • headers have been set differently across exporters, sometimes overriding what the base constructor does, this change also streamlines how headers are set by the exporters by requiring them to be passed to the base-class constructor.
  • removes the compression property to reduce the API surface of the OTLPExporterNodeBase and its child-classes

A follow-up will do the same for browser transports, eventually removing the need to have different exporter-bases and retry implementations for Node.js and the Browser. This follow-up will conclude the tasks defined in #4116 and enabling implementation of feature-requests like #4631

Another follow-up will address config, splitting the config defaults and env-var code from the actual exporters. Then the tests will be adapted so that changes to underlying structures don't have to be propagated to the signal-/transport-specific exporters.

Breaking changes:

  • (user-facing) headers was intended for internal use has been removed from all exporters
  • (user-facing) compression was intended for internal use and has been removed from all exporters
  • (user-facing) hostname was intended for use in tests and is not used by any exporters, it will be removed in a future
  • (internal) OTLPExporterNodeBase constructor has changed and now expects singalSpecificHeaders to be passed to the constructor.

Updates #4116

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Adapted existing tests
  • Added Unit tests

@pichlermarc pichlermarc changed the title Feat/node exporter transport feat(exporters)!: use transport interface in node.js exporters May 31, 2024
Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.53%. Comparing base (ecc88a3) to head (b039d49).
Report is 45 commits behind head on main.

Current head b039d49 differs from pull request most recent head f1577c0

Please upload reports for the commit f1577c0 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4743      +/-   ##
==========================================
+ Coverage   91.04%   93.53%   +2.49%     
==========================================
  Files          89      248     +159     
  Lines        1954     5789    +3835     
  Branches      416     1161     +745     
==========================================
+ Hits         1779     5415    +3636     
- Misses        175      374     +199     

see 161 files with indirect coverage changes

@pichlermarc pichlermarc marked this pull request as ready for review June 3, 2024 14:40
@pichlermarc pichlermarc requested a review from a team June 3, 2024 14:40
getEnv().OTEL_EXPORTER_OTLP_LOGS_HEADERS
),
...parseHeaders(config?.headers),
...USER_AGENT,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this may change when #4447 is solved. Could you please add a reference here?

Copy link
Member Author

@pichlermarc pichlermarc Jun 21, 2024

Choose a reason for hiding this comment

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

@david-luna, sorry I think I've lost the train of thought from #4447.

Would you like me to add a reference that precedence may change

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a note in the CHANGELOG.md: bb5ec8c

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @pichlermarc !

Once the exporters are finalized I'll check other SDKs and ask in the spec SIG about #4447 and the possibility to override the UA via env vars.

while (result.status === 'retryable' && attempts > 0) {
attempts--;
const upperBound = Math.min(nextBackoff, MAX_BACKOFF);
const backoff = Math.random() * upperBound;
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this is an iterative way of implementing exponential backoff (with 1,5 as exponent). What I do not fully get is this multiplication with Math.rand() 🤔

With this randomness in place the waiting time between retries may not increase. We may even get small values consuming the reties very fast.

  • Maybe we need also a lowerBound?
  • Or maybe its even simpler if just use Math.min(nextBackoff, MAX_BACKOFF)? The progression is [1000, 1500, 2250, 3375, 5000, 5000 ...]

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes you're right, that does not make any sense. 🤦 I think I was trying to implement a jitter but then failed to follow through on it and ended up with this thing. I think I fixed it in in f1577c0.

It takes the min of nextBackoff and MAX_BACKOFF and then adds some jitter [-0.2ms, +0.2ms] to it. Would appreciate you having another look 👀

@pichlermarc pichlermarc added this pull request to the merge queue Jul 29, 2024
Merged via the queue into open-telemetry:main with commit d91dbe1 Jul 29, 2024
19 checks passed
@pichlermarc pichlermarc deleted the feat/node-exporter-transport branch July 29, 2024 14:54
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…telemetry#4743)

* feat(exporters)!: use transport interface in node.js exporters

* feat(exporters): hide compression property

* feat(otlp-exporter-base)!: remove header property

* feat(otlp-exporter-base): add retrying transport

* fix: lint

* chore: add changelog entry

* fix: use queueMicrotask over nextTick

* chore: move changelog entry to unreleased

* chore: note that user-agent cannot be overwritten by users anymore

* fix: export missing ExportResponseRetryable

* fix: retry jitter
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