feat(exporter-otlp-*)!: support custom HTTP agents#5719
feat(exporter-otlp-*)!: support custom HTTP agents#5719pichlermarc merged 16 commits intoopen-telemetry:mainfrom
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:
|
|
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
|
@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 |
|
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 :) |
pichlermarc
left a comment
There was a problem hiding this comment.
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));
-}
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
pichlermarc
left a comment
There was a problem hiding this comment.
Looks good to merge, just some nits around docs and such :)
Thanks for your patience :)
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
|
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 |
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Which problem is this PR solving?
This makes it possible to use a custom
http.Agentwith 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
httpAgentOptionsoption 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 thehttpmodule and avoid issues where it is loaded before being instrumented.The dynamic
requireofhttp-transport-utilshas also been migrated to dynamicimport, which should make it possible to bundle with Rollup. This works in tests by overriding thets-nodeoptions so that theimportcall 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: