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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 18 commits into from
Jul 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
fb97582
feat(exporters)!: use transport interface in node.js exporters
pichlermarc May 8, 2024
cf6f479
feat(exporters): hide compression property
pichlermarc May 8, 2024
4d785ce
feat(otlp-exporter-base)!: remove header property
pichlermarc May 17, 2024
e8635df
feat(otlp-exporter-base): add retrying transport
pichlermarc May 31, 2024
b039d49
fix: lint
pichlermarc May 31, 2024
c284ec4
chore: add changelog entry
pichlermarc May 31, 2024
084a893
Merge branch 'main' into feat/node-exporter-transport
pichlermarc Jun 3, 2024
6127207
Merge branch 'main' into feat/node-exporter-transport
pichlermarc Jun 6, 2024
56a93de
Merge branch 'main' into feat/node-exporter-transport
pichlermarc Jun 10, 2024
b19734e
fix: use queueMicrotask over nextTick
pichlermarc Jun 12, 2024
4df0efa
chore: move changelog entry to unreleased
pichlermarc Jun 12, 2024
744c5f6
Merge branch 'main' into feat/node-exporter-transport
pichlermarc Jun 12, 2024
bb5ec8c
chore: note that user-agent cannot be overwritten by users anymore
pichlermarc Jun 21, 2024
767eedc
fix: export missing ExportResponseRetryable
pichlermarc Jun 21, 2024
f1577c0
fix: retry jitter
pichlermarc Jun 21, 2024
a7f73a9
Merge branch 'main' into feat/node-exporter-transport
pichlermarc Jul 4, 2024
a071618
Merge branch 'main' into feat/node-exporter-transport
pichlermarc Jul 29, 2024
0ad653e
Merge branch 'main' into feat/node-exporter-transport
pichlermarc Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ All notable changes to experimental packages in this project will be documented
### :boom: Breaking Change

* fix(instrumentation)!:remove unused description property from interface [#4847](https://github.com/open-telemetry/opentelemetry-js/pull/4847) @blumamir
* feat(exporter-*-otlp-*)!: use transport interface in node.js exporters [#4743](https://github.com/open-telemetry/opentelemetry-js/pull/4743) @pichlermarc
* (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 release
* fix(exporter-*-otlp-*)!: ensure `User-Agent` header cannot be overwritten by the user [#4743](https://github.com/open-telemetry/opentelemetry-js/pull/4743) @pichlermarc
* allowing overrides of the `User-Agent` header was not specification compliant.

### :rocket: (Enhancement)

Expand Down Expand Up @@ -79,7 +85,7 @@ All notable changes to experimental packages in this project will be documented

### :bug: (Bug Fix)

* fix(instrumentation): Update `import-in-the-middle` to fix [numerous bugs](https://github.com/DataDog/import-in-the-middle/releases/tag/v1.8.0) [#4745](https://github.com/open-telemetry/opentelemetry-js/pull/4745) @timfish
* fix(instrumentation): Update `import-in-the-middle` to fix [numerous bugs](https://github.com/DataDog/import-in-the-middle/pull/91) [#4745](https://github.com/open-telemetry/opentelemetry-js/pull/4745) @timfish

### :books: (Refine Doc)

Expand Down
17 changes: 8 additions & 9 deletions experimental/packages/exporter-logs-otlp-http/src/platform/node/OTLPLogExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,15 @@ export class OTLPLogExporter
...config,
},
JsonLogsSerializer,
'application/json'
{
...baggageUtils.parseKeyPairsIntoRecord(
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 •
edited
Loading

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.

'Content-Type': 'application/json',
}
);
this.headers = {
...this.headers,
...USER_AGENT,
...baggageUtils.parseKeyPairsIntoRecord(
getEnv().OTEL_EXPORTER_OTLP_LOGS_HEADERS
),
...parseHeaders(config?.headers),
};
}

getDefaultUrl(config: OTLPExporterNodeConfigBase): string {
Expand Down
79 changes: 53 additions & 26 deletions experimental/packages/exporter-logs-otlp-http/test/node/OTLPLogExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class MockedResponse extends Stream {
super();
}

send(data: string) {
send(data: Uint8Array) {
this.emit('data', data);
this.emit('end');
}
Expand All @@ -65,6 +65,9 @@ describe('OTLPLogExporter', () => {

afterEach(() => {
fakeRequest = new Stream.PassThrough();
Object.defineProperty(fakeRequest, 'setTimeout', {
value: function (_timeout: number) {},
});
sinon.restore();
});

Expand All @@ -83,15 +86,20 @@ describe('OTLPLogExporter', () => {
it('should include user-agent header by default', () => {
const exporter = new OTLPLogExporter();
assert.strictEqual(
exporter.headers['User-Agent'],
exporter['_transport']['_transport']['_parameters']['headers'][
'User-Agent'
],
`OTel-OTLP-Exporter-JavaScript/${VERSION}`
);
});

it('should use headers defined via env', () => {
envSource.OTEL_EXPORTER_OTLP_LOGS_HEADERS = 'foo=bar';
const exporter = new OTLPLogExporter();
assert.strictEqual(exporter.headers.foo, 'bar');
assert.strictEqual(
exporter['_transport']['_transport']['_parameters']['headers']['foo'],
'bar'
);
delete envSource.OTEL_EXPORTER_OTLP_LOGS_HEADERS;
});

Expand All @@ -106,13 +114,19 @@ describe('OTLPLogExporter', () => {

it('should override headers defined via env with headers defined in constructor', () => {
envSource.OTEL_EXPORTER_OTLP_HEADERS = 'foo=bar,bar=foo';
const collectorExporter = new OTLPLogExporter({
const exporter = new OTLPLogExporter({
headers: {
foo: 'constructor',
},
});
assert.strictEqual(collectorExporter.headers.foo, 'constructor');
assert.strictEqual(collectorExporter.headers.bar, 'foo');
assert.strictEqual(
exporter['_transport']['_transport']['_parameters']['headers']['foo'],
'constructor'
);
assert.strictEqual(
exporter['_transport']['_transport']['_parameters']['headers']['bar'],
'foo'
);
envSource.OTEL_EXPORTER_OTLP_HEADERS = '';
});
});
Expand Down Expand Up @@ -152,10 +166,12 @@ describe('OTLPLogExporter', () => {
assert.strictEqual(options.method, 'POST');
assert.strictEqual(options.path, '/');

const mockRes = new MockedResponse(200);
cb(mockRes);
mockRes.send('success');
done();
queueMicrotask(() => {
const mockRes = new MockedResponse(200);
cb(mockRes);
mockRes.send(Buffer.from('success'));
done();
});
return fakeRequest as any;
});
collectorExporter.export(logs, () => {});
Expand All @@ -165,10 +181,12 @@ describe('OTLPLogExporter', () => {
sinon.stub(http, 'request').callsFake((options: any, cb: any) => {
assert.strictEqual(options.headers['foo'], 'bar');

const mockRes = new MockedResponse(200);
cb(mockRes);
mockRes.send('success');
done();
queueMicrotask(() => {
const mockRes = new MockedResponse(200);
cb(mockRes);
mockRes.send(Buffer.from('success'));
done();
});
return fakeRequest as any;
});

Expand All @@ -180,10 +198,12 @@ describe('OTLPLogExporter', () => {
assert.strictEqual(options.agent.keepAlive, true);
assert.strictEqual(options.agent.options.keepAliveMsecs, 2000);

const mockRes = new MockedResponse(200);
cb(mockRes);
mockRes.send('success');
done();
queueMicrotask(() => {
const mockRes = new MockedResponse(200);
cb(mockRes);
mockRes.send(Buffer.from('success'));
done();
});
return fakeRequest as any;
});

Expand All @@ -192,10 +212,13 @@ describe('OTLPLogExporter', () => {

it('should successfully send the logs', done => {
const fakeRequest = new Stream.PassThrough();
sinon.stub(http, 'request').returns(fakeRequest as any);
Object.defineProperty(fakeRequest, 'setTimeout', {
value: function (_timeout: number) {},
});

sinon.stub(http, 'request').returns(fakeRequest as any);
let buff = Buffer.from('');
fakeRequest.on('end', () => {
fakeRequest.on('finish', () => {
const responseBody = buff.toString();
const json = JSON.parse(responseBody) as IExportLogsServiceRequest;
const log1 = json.resourceLogs?.[0].scopeLogs?.[0].logRecords?.[0];
Expand All @@ -222,9 +245,11 @@ describe('OTLPLogExporter', () => {
const spyLoggerError = sinon.stub(diag, 'error');

sinon.stub(http, 'request').callsFake((options: any, cb: any) => {
const mockRes = new MockedResponse(200);
cb(mockRes);
mockRes.send('success');
queueMicrotask(() => {
const mockRes = new MockedResponse(200);
cb(mockRes);
mockRes.send(Buffer.from('success'));
});
return fakeRequest as any;
});

Expand All @@ -237,9 +262,11 @@ describe('OTLPLogExporter', () => {

it('should log the error message', done => {
sinon.stub(http, 'request').callsFake((options: any, cb: any) => {
const mockResError = new MockedResponse(400);
cb(mockResError);
mockResError.send('failed');
queueMicrotask(() => {
const mockRes = new MockedResponse(400);
cb(mockRes);
mockRes.send(Buffer.from('failure'));
});

return fakeRequest as any;
});
Expand Down
12 changes: 5 additions & 7 deletions experimental/packages/exporter-logs-otlp-proto/src/platform/node/OTLPLogExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,14 @@ export class OTLPLogExporter
implements LogRecordExporter
{
constructor(config: OTLPExporterConfigBase = {}) {
super(config, ProtobufLogsSerializer, 'application/x-protobuf');
const env = getEnv();
this.headers = {
...this.headers,
...USER_AGENT,
super(config, ProtobufLogsSerializer, {
...baggageUtils.parseKeyPairsIntoRecord(
env.OTEL_EXPORTER_OTLP_LOGS_HEADERS
getEnv().OTEL_EXPORTER_OTLP_LOGS_HEADERS
),
...parseHeaders(config?.headers),
};
...USER_AGENT,
'Content-Type': 'application/x-protobuf',
});
}

getDefaultUrl(config: OTLPExporterConfigBase): string {
Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/exporter-logs-otlp-proto/test/logHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ export class MockedResponse extends Stream {
super();
}

send(data: string) {
send(data: Uint8Array) {
this.emit('data', data);
this.emit('end');
}
Expand Down
Loading