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(otlp-exporter-base): allow agent override in Node exporter #3935

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

henrinormak
Copy link

@henrinormak henrinormak commented Jun 22, 2023

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

  • 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:

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

@henrinormak henrinormak requested a review from a team June 22, 2023 06:24
@codecov
Copy link

codecov bot commented Jun 22, 2023

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%.

❗ 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

@github-actions
Copy link

github-actions bot commented Sep 4, 2023

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 github-actions bot added the stale label Sep 4, 2023
@henrinormak
Copy link
Author

henrinormak commented Sep 4, 2023

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

@pichlermarc
Copy link
Member

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. 🤔

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.

Copy link

github-actions bot commented Jan 8, 2024

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 github-actions bot added the stale label Jan 8, 2024
@github-actions github-actions bot removed the stale label Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants