-
Notifications
You must be signed in to change notification settings - Fork 924
feat(exporter-otlp-*)!: support custom HTTP agents #5719
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(exporter-otlp-*)!: support custom HTTP agents #5719
Conversation
af65d9f
to
7d4481b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5719 +/- ##
=======================================
Coverage 95.04% 95.04%
=======================================
Files 307 307
Lines 7986 7993 +7
Branches 1614 1614
=======================================
+ Hits 7590 7597 +7
Misses 396 396
🚀 New features to boost your workflow:
|
experimental/packages/otlp-exporter-base/src/transport/http-transport-types.ts
Outdated
Show resolved
Hide resolved
Updated to use a factory function for the agent, this iteration definitely feels cleaner than the previous one. I was also able to make what you attempted with #5220 work with the tests @pichlermarc (except now it breaks the instrumentation tests, looking into that) |
59b612a
to
06fa08b
Compare
@raphael-theriault-swi nice 🙂 Before doing another round of reviews: it looks like the API tests are now failing here due to the changed settings: opentelemetry-js/api/test/common/internal/version.test.ts Lines 27 to 28 in a4e9645
|
5ee1842
to
58f9655
Compare
experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts
Outdated
Show resolved
Hide resolved
@pichlermarc I'm looking into it, the failure is not reproducible on macOS (with Node 20.6.0) so it'll have to wait a bit until next week when I have time to set up an environment where it also fails |
experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts
Outdated
Show resolved
Hide resolved
This PR is currently on hold as there's no way to make the tests work with |
7c9eae4
to
3166b11
Compare
@pichlermarc I've been banging my head for a week trying to migrate the tests to use something other than |
@pichlermarc would appreciate a review when you have some time :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good already, thank you for working on this. 🙂
By looking at the way you solved the lazy-loading of the Agent
I noticed that we could apply something similar to get rid of lazy-loading the full http-transport-util.ts
file by just lazy-loading the request
function instead.
That allows us to undo some of the changes to the mocharc
and tsconfig
files, leaving configs a bit cleaner and also gets rid of the awkward file lazy-loading that causes everyone trouble:
diff --git a/.mocharc.js b/.mocharc.js
deleted file mode 100644
index 11f4a7d8c..000000000
--- a/.mocharc.js
+++ /dev/null
@@ -1,3 +0,0 @@
-module.exports = {
- require: `${__dirname}/ts-node.js`,
-};
diff --git a/.mocharc.yml b/.mocharc.yml
new file mode 100644
index 000000000..45bd4bdc1
--- /dev/null
+++ b/.mocharc.yml
@@ -0,0 +1 @@
+require: 'ts-node/register'
diff --git a/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts b/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts
index 05d45d6dc..c75d0cd7d 100644
--- a/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts
+++ b/experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts
@@ -14,10 +14,7 @@
* limitations under the License.
*/
-import type {
- HttpRequestParameters,
- sendWithHttp,
-} from './http-transport-types';
+import type { HttpRequestParameters } from './http-transport-types';
// NOTE: do not change these type imports to actual imports. Doing so WILL break `@opentelemetry/instrumentation-http`,
// as they'd be imported before the http/https modules can be wrapped.
@@ -25,10 +22,11 @@ import type * as https from 'https';
import type * as http from 'http';
import { ExportResponse } from '../export-response';
import { IExporterTransport } from '../exporter-transport';
+import { sendWithHttp } from './http-transport-utils';
interface Utils {
agent: http.Agent | https.Agent;
- send: sendWithHttp;
+ request: typeof http.request | typeof https.request;
}
class HttpExporterTransport implements IExporterTransport {
@@ -37,10 +35,11 @@ class HttpExporterTransport implements IExporterTransport {
constructor(private _parameters: HttpRequestParameters) {}
async send(data: Uint8Array, timeoutMillis: number): Promise<ExportResponse> {
- const { agent, send } = await this._loadUtils();
+ const { agent, request } = await this._loadUtils();
return new Promise<ExportResponse>(resolve => {
- send(
+ sendWithHttp(
+ request,
this._parameters,
agent,
data,
@@ -60,16 +59,10 @@ class HttpExporterTransport implements IExporterTransport {
let utils = this._utils;
if (utils === null) {
- // Lazy import to ensure that http/https is not required before instrumentations can wrap it.
- const imported = await import('./http-transport-utils.js');
-
+ const protocol = new URL(this._parameters.url).protocol;
utils = this._utils = {
- agent: await this._parameters.agent(
- new URL(this._parameters.url).protocol
- ),
- // @ts-expect-error dynamic imports are never transpiled, but named exports only work when
- // dynamically importing an esm file, and the utils file might be transpiled to cjs
- send: imported.sendWithHttp ?? imported.default.sendWithHttp,
+ agent: await this._parameters.agent(protocol),
+ request: await getRequestFunction(protocol),
};
}
@@ -77,6 +70,18 @@ class HttpExporterTransport implements IExporterTransport {
}
}
+async function getRequestFunction(
+ protocol: string
+): Promise<typeof http.request | typeof https.request> {
+ if (protocol === 'http:') {
+ const { request } = await import('http');
+ return request;
+ } else {
+ const { request } = await import('https');
+ return request;
+ }
+}
+
export function createHttpExporterTransport(
parameters: HttpRequestParameters
): IExporterTransport {
diff --git a/experimental/packages/otlp-exporter-base/src/transport/http-transport-types.ts b/experimental/packages/otlp-exporter-base/src/transport/http-transport-types.ts
index 5c9797bc9..11a982c1a 100644
--- a/experimental/packages/otlp-exporter-base/src/transport/http-transport-types.ts
+++ b/experimental/packages/otlp-exporter-base/src/transport/http-transport-types.ts
@@ -14,19 +14,8 @@
* limitations under the License.
*/
-import type * as http from 'http';
-import type * as https from 'https';
-import { ExportResponse } from '../export-response';
import { HttpAgentFactory } from '../configuration/otlp-http-configuration';
-export type sendWithHttp = (
- params: HttpRequestParameters,
- agent: http.Agent | https.Agent,
- data: Uint8Array,
- onDone: (response: ExportResponse) => void,
- timeoutMillis: number
-) => void;
-
export interface HttpRequestParameters {
url: string;
headers: () => Record<string, string>;
diff --git a/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts b/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts
index 331d70757..5aa00e5eb 100644
--- a/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts
+++ b/experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts
@@ -13,8 +13,8 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import * as http from 'http';
-import * as https from 'https';
+import type * as http from 'http';
+import type * as https from 'https';
import * as zlib from 'zlib';
import { Readable } from 'stream';
import { HttpRequestParameters } from './http-transport-types';
@@ -27,6 +27,7 @@ import { OTLPExporterError } from '../types';
/**
* Sends data using http
+ * @param requestFunction
* @param params
* @param agent
* @param data
@@ -34,6 +35,7 @@ import { OTLPExporterError } from '../types';
* @param timeoutMillis
*/
export function sendWithHttp(
+ requestFunction: typeof https.request | typeof http.request,
params: HttpRequestParameters,
agent: http.Agent | https.Agent,
data: Uint8Array,
@@ -53,9 +55,7 @@ export function sendWithHttp(
agent: agent,
};
- const request = parsedUrl.protocol === 'http:' ? http.request : https.request;
-
- const req = request(options, (res: http.IncomingMessage) => {
+ const req = requestFunction(options, (res: http.IncomingMessage) => {
const responseData: Buffer[] = [];
res.on('data', chunk => responseData.push(chunk));
diff --git a/experimental/packages/otlp-exporter-base/tsconfig.json b/experimental/packages/otlp-exporter-base/tsconfig.json
index 51b04951d..2bff90df9 100644
--- a/experimental/packages/otlp-exporter-base/tsconfig.json
+++ b/experimental/packages/otlp-exporter-base/tsconfig.json
@@ -19,12 +19,5 @@
{
"path": "../otlp-transformer"
}
- ],
- "ts-node": {
- "experimentalResolver": true,
- "compilerOptions": {
- "module": "commonjs",
- "moduleResolution": "node10"
- }
- }
+ ]
}
diff --git a/ts-node.js b/ts-node.js
deleted file mode 100644
index f1f97346e..000000000
--- a/ts-node.js
+++ /dev/null
@@ -1,8 +0,0 @@
-require('ts-node/register');
-
-if (process.env.MOCHA_PATCH_ESM != null) {
- const { register } = require('module');
- const { pathToFileURL } = require('url');
-
- register('ts-node/esm', pathToFileURL(__filename));
-}
experimental/packages/otlp-exporter-base/src/configuration/convert-legacy-node-http-options.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Marc Pichler <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to merge, just some nits around docs and such :)
Thanks for your patience :)
experimental/packages/otlp-exporter-base/src/configuration/legacy-node-configuration.ts
Show resolved
Hide resolved
Co-authored-by: Marc Pichler <[email protected]>
Updated the title at the same time for the breaking change |
Not sure what is happening here given the commits are in fact linked to my account |
This comment was marked as resolved.
This comment was marked as resolved.
I can try and rebase without the co-author comments given that's what seems to break it |
/easycla |
sorry folks, see open-telemetry/community#2920 (comment), we've rolled back the change for now, should be all good here |
Which problem is this PR solving?
This makes it possible to use a custom
http.Agent
with the exporters, which enables some useful use cases, mainly proxying, without having to implement them in the SDK.Fixes #5711
Short description of the changes
The
httpAgentOptions
option now also accepts an HTTP agent factory function. All internal code is migrated to use the factory; if an options object is provided, it is converted to a function. The function is only called once an agent is needed for export, which makes it possible to lazy import thehttp
module and avoid issues where it is loaded before being instrumented.The dynamic
require
ofhttp-transport-utils
has also been migrated to dynamicimport
, which should make it possible to bundle with Rollup. This works in tests by overriding thets-node
options so that theimport
call gets transpiled torequire
(only for tests) which loads fine.Type of change
How Has This Been Tested?
Added a test to make sure the custom agent is in fact being used.
Checklist: