opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

feat(otlp-exporter-base): allow agent override in Node exporter

Open henrinormak opened this issue 2 years ago • 5 comments

Which problem is this PR solving?

Currently, the OTLP HTTP exporters in Node environments only allow providing a configuration for the underlying Agent used when making the requests, however, there are instances in which one might want to opt-out of using the default node http/https Agent, and prefer using something like agentkeepalive. One of these for example is in order to enforce a TTL on the sockets, which does not seem doable with the current setup.

Short description of the changes

Allow providing an Agent directly, instead of only providing a way of providing a configuration for the agent. This comes as a change to the OTLPExporterNodeBase class and its' configuration type.

Type of change

  • [X] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Currently not covered, looking for feedback on how this could be covered. One possibility is to do it via the various extensions on the base class, i.e OTLPTraceExporter?

Checklist:

  • [X] Followed the style guidelines of this project
  • [ ] Unit tests have been added
  • [X] Documentation has been updated (Updated the types)

henrinormak avatar Jun 22 '23 06:06 henrinormak

Codecov Report

Merging #3935 (b68f8f6) into main (de5cd0f) will decrease coverage by 0.04%. Report is 164 commits behind head on main. The diff coverage is 60.00%.

:exclamation: Current head b68f8f6 differs from pull request most recent head 334786d. Consider uploading reports for the commit 334786d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3935      +/-   ##
==========================================
- Coverage   93.14%   93.11%   -0.04%     
==========================================
  Files         298      298              
  Lines        8869     8873       +4     
  Branches     1826     1830       +4     
==========================================
+ Hits         8261     8262       +1     
- Misses        608      611       +3     
Files Coverage Δ
...ages/otlp-exporter-base/src/platform/node/types.ts 100.00% <ø> (ø)
...ter-base/src/platform/node/OTLPExporterNodeBase.ts 50.00% <60.00%> (ø)

... and 1 file with indirect coverage changes

codecov[bot] avatar Jun 22 '23 13:06 codecov[bot]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Sep 04 '23 06:09 github-actions[bot]

I still would like to see this, so commenting to remove stale

henrinormak avatar Sep 04 '23 07:09 henrinormak

I can see that this feature could be helpful, though I'm hesitant to accept new features to the OTLP exporters before some extensive refactoring is done on them. :thinking:

There was a proposal a very long time ago to introduce a transport layer to the exporters (#422). I've opened another issue (#4116) to track this in a more recent context, as #422 was created when OTLP exporters did not exist yet. I believe that we need to take care of this first, as we're increasingly adding features to the OTLP exporters while having difficulty in adequately testing them.

@open-telemetry/javascript-maintainers wdyt? IMO, we should feature-freeze OTLP exporters until this refactoring is done.

pichlermarc avatar Sep 04 '23 11:09 pichlermarc

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Jan 08 '24 06:01 github-actions[bot]