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!: move serialization to @opentelemetry/otlp-transformer #4542

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
fc9cb4c
feat!:move serializers to otlp-transformer
pichlermarc Mar 13, 2024
dfffc23
feat!: use serializeres in protobuf and json exporters
pichlermarc Mar 13, 2024
79256ed
Merge branch 'main' into feat/transformer-serializer
pichlermarc Mar 14, 2024
76c4667
test(otlp-transformer): add tests for trace serializer
pichlermarc Mar 15, 2024
c14bc9b
test(otlp-transformer): add tests for metrics serializer
pichlermarc Mar 15, 2024
432cb41
test(otlp-transformer): add tests for logs serializer
pichlermarc Mar 20, 2024
b98c061
Merge branch 'main' into feat/transformer-serializer
pichlermarc Mar 21, 2024
5ebc27c
Merge branch 'main' into feat/transformer-serializer
pichlermarc Mar 25, 2024
12c2ad2
Merge branch 'main' into feat/transformer-serializer
pichlermarc Mar 26, 2024
ac2d1bd
Merge branch 'main' into feat/transformer-serializer
pichlermarc Mar 29, 2024
bb88491
Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 2, 2024
0ff7bf6
Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 3, 2024
9a9c346
Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 10, 2024
5205cae
chore: resolve more conflicts
pichlermarc Apr 10, 2024
05640f9
Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 11, 2024
85c3f1e
Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 11, 2024
b9bdeea
fix: sync package-lock
pichlermarc Apr 11, 2024
fd77885
chore: cleanup dependencies, unused code, .gitignore
pichlermarc Apr 15, 2024
105da17
chore: fix changelog indentation
pichlermarc Apr 15, 2024
9cb2593
fix(otlp-transformer): remove unused useHex from JsonMetricsSerializer
pichlermarc Apr 15, 2024
4f80ada
chore: add comment about how logs data is structured
pichlermarc Apr 15, 2024
e8adc4d
docs: move submodule.md, adapt contents
pichlermarc Apr 15, 2024
998cf96
Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 15, 2024
07c7170
Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 26, 2024
62cc3b3
fixup! Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 26, 2024
1324071
fixup! Merge branch 'main' into feat/transformer-serializer
pichlermarc Apr 26, 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
6 changes: 3 additions & 3 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[submodule "experimental/packages/otlp-grpc-exporter-base/protos"]
path = experimental/packages/otlp-grpc-exporter-base/protos
url = https://github.com/open-telemetry/opentelemetry-proto.git
[submodule "experimental/packages/otlp-proto-exporter-base/protos"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we'll be able to remove this submodule here too in the follow-up to do the same to the browser exporters.

path = experimental/packages/otlp-proto-exporter-base/protos
url = https://github.com/open-telemetry/opentelemetry-proto.git
[submodule "experimental/packages/otlp-transformer/protos"]
path = experimental/packages/otlp-transformer/protos
url = https://github.com/open-telemetry/opentelemetry-proto.git
6 changes: 6 additions & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ All notable changes to experimental packages in this project will be documented

### :boom: Breaking Change

* feat(exporter-*-otlp-*)!: move serialization for Node.js exporters to `@opentelemetry/otlp-transformer` [#4542](https://github.com/open-telemetry/opentelemetry-js/pull/4542) @pichlermarc
* Breaking changes:
* (user-facing) `convert()` now returns an empty object and will be removed in a follow-up
* (internal) OTLPExporterNodeBase now has additional constructor parameters that are required
* (internal) OTLPExporterNodeBase now has an additional `ResponseType` type parameter

### :rocket: (Enhancement)

### :bug: (Bug Fix)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,11 @@ import {
OTLPGRPCExporterNodeBase,
validateAndNormalizeUrl,
DEFAULT_COLLECTOR_URL,
LogsSerializer,
} from '@opentelemetry/otlp-grpc-exporter-base';
import {
createExportLogsServiceRequest,
IExportLogsServiceRequest,
IExportLogsServiceResponse,
ProtobufLogsSerializer,
} from '@opentelemetry/otlp-transformer';
import { VERSION } from './version';

Expand Down Expand Up @@ -57,14 +56,10 @@ export class OTLPLogExporter
signalSpecificMetadata,
'LogsExportService',
'/opentelemetry.proto.collector.logs.v1.LogsService/Export',
LogsSerializer
ProtobufLogsSerializer
);
}

convert(logRecords: ReadableLogRecord[]): IExportLogsServiceRequest {
return createExportLogsServiceRequest(logRecords);
}

getDefaultUrl(config: OTLPGRPCExporterConfigNode) {
return validateAndNormalizeUrl(this.getUrlFromConfig(config));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@ import { VERSION } from '../src/version';

const logsServiceProtoPath =
'opentelemetry/proto/collector/logs/v1/logs_service.proto';
const includeDirs = [
path.resolve(__dirname, '../../otlp-grpc-exporter-base/protos'),
];
const includeDirs = [path.resolve(__dirname, '../../otlp-transformer/protos')];

const httpAddr = 'https://localhost:1503';
const udsAddr = 'unix:///tmp/otlp-logs.sock';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@ import type {
LogRecordExporter,
} from '@opentelemetry/sdk-logs';
import type { OTLPExporterNodeConfigBase } from '@opentelemetry/otlp-exporter-base';
import type { IExportLogsServiceRequest } from '@opentelemetry/otlp-transformer';
import type {
IExportLogsServiceRequest,
IExportLogsServiceResponse,
} from '@opentelemetry/otlp-transformer';
import { getEnv, baggageUtils } from '@opentelemetry/core';
import {
OTLPExporterNodeBase,
parseHeaders,
} from '@opentelemetry/otlp-exporter-base';
import { createExportLogsServiceRequest } from '@opentelemetry/otlp-transformer';
import { JsonLogsSerializer } from '@opentelemetry/otlp-transformer';

import { getDefaultUrl } from '../config';
import { VERSION } from '../../version';
Expand All @@ -38,15 +41,23 @@ const USER_AGENT = {
* Collector Logs Exporter for Node
*/
export class OTLPLogExporter
extends OTLPExporterNodeBase<ReadableLogRecord, IExportLogsServiceRequest>
extends OTLPExporterNodeBase<
ReadableLogRecord,
IExportLogsServiceRequest,
IExportLogsServiceResponse
>
implements LogRecordExporter
{
constructor(config: OTLPExporterNodeConfigBase = {}) {
// load OTEL_EXPORTER_OTLP_LOGS_TIMEOUT env
super({
timeoutMillis: getEnv().OTEL_EXPORTER_OTLP_LOGS_TIMEOUT,
...config,
});
super(
{
timeoutMillis: getEnv().OTEL_EXPORTER_OTLP_LOGS_TIMEOUT,
...config,
},
JsonLogsSerializer,
'application/json'
);
this.headers = {
...this.headers,
...USER_AGENT,
Expand All @@ -57,13 +68,6 @@ export class OTLPLogExporter
};
}

convert(logRecords: ReadableLogRecord[]): IExportLogsServiceRequest {
return createExportLogsServiceRequest(logRecords, {
useHex: true,
useLongBits: false,
});
}

getDefaultUrl(config: OTLPExporterNodeConfigBase): string {
return getDefaultUrl(config);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,14 @@ import {
OTLPExporterConfigBase,
appendResourcePathToUrl,
appendRootPathToUrlIfNeeded,
OTLPExporterNodeBase,
parseHeaders,
} from '@opentelemetry/otlp-exporter-base';
import { ServiceClientType } from '@opentelemetry/otlp-proto-exporter-base';
import {
OTLPProtoExporterNodeBase,
ServiceClientType,
} from '@opentelemetry/otlp-proto-exporter-base';
import {
createExportLogsServiceRequest,
IExportLogsServiceRequest,
IExportLogsServiceResponse,
ProtobufLogsSerializer,
} from '@opentelemetry/otlp-transformer';

import { ReadableLogRecord, LogRecordExporter } from '@opentelemetry/sdk-logs';
Expand All @@ -44,14 +43,15 @@ const DEFAULT_COLLECTOR_URL = `http://localhost:4318/${DEFAULT_COLLECTOR_RESOURC
* Collector Trace Exporter for Node
*/
export class OTLPLogExporter
extends OTLPProtoExporterNodeBase<
extends OTLPExporterNodeBase<
ReadableLogRecord,
IExportLogsServiceRequest
IExportLogsServiceRequest,
IExportLogsServiceResponse
>
implements LogRecordExporter
{
constructor(config: OTLPExporterConfigBase = {}) {
super(config);
super(config, ProtobufLogsSerializer, 'application/x-protobuf');
this.headers = {
...this.headers,
...USER_AGENT,
Expand All @@ -61,9 +61,6 @@ export class OTLPLogExporter
...parseHeaders(config?.headers),
};
}
convert(logs: ReadableLogRecord[]): IExportLogsServiceRequest {
return createExportLogsServiceRequest(logs);
}

getDefaultUrl(config: OTLPExporterConfigBase): string {
return typeof config.url === 'string'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,6 @@ describe('OTLPLogExporter - node with proto over http', () => {
});

it('should open the connection', done => {
collectorExporter.export(logs, () => {});

sinon.stub(http, 'request').callsFake((options: any, cb: any) => {
assert.strictEqual(options.hostname, 'foo.bar.com');
assert.strictEqual(options.method, 'POST');
Expand All @@ -206,11 +204,10 @@ describe('OTLPLogExporter - node with proto over http', () => {
done();
return fakeRequest as any;
});
collectorExporter.export(logs, () => {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with these tests. Is it necessary to move this call at the bottom of the test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question - I should've put a note here as it's quite odd 🙂

I removed the OTLPProtoExporterNodeBase class and the proto exporters are now using the same export logic as the JSON exporter (OTLPExporterNodeBase). That means is that it does not use the setImmediate() anymore to delay the export (see here for code that's used now). As this setImmedidate() was there before, it was possible call export, then mock and wait for the test to finish. As it's gone, that does not work anymore (export() will actually export when it's called, not in the next tick).

From what I can tell the setImmediate() was added to OTLPProtoExporterNodeBase to avoid loading http before it can be instrumented (the comment mentioned protobufjs, but AFAIK there's no such instrumentation). While this was a good effort to try and delay the loading of the http module, OTLPProtoExporterNodeBase extended OTLPExporterNodeBase which made no effort to delay the loading of http. This made the proto-base's efforts to avoid loading ineffective, as it meant that http was imported when the base was required regardless of the setImmediate() in the OTLPProtoExporterNodeBase.

In follow-ups, I'll re-structure the transport code in the exporters to use the code from #4415. This will then actually implement the lazy-loading that was intended to be there in the first place.

});

it('should set custom headers', done => {
collectorExporter.export(logs, () => {});

sinon.stub(http, 'request').callsFake((options: any, cb: any) => {
assert.strictEqual(options.headers['foo'], 'bar');

Expand All @@ -220,11 +217,10 @@ describe('OTLPLogExporter - node with proto over http', () => {
done();
return fakeRequest as any;
});
collectorExporter.export(logs, () => {});
});

it('should have keep alive and keepAliveMsecs option set', done => {
collectorExporter.export(logs, () => {});

sinon.stub(http, 'request').callsFake((options: any, cb: any) => {
assert.strictEqual(options.agent.keepAlive, true);
assert.strictEqual(options.agent.options.keepAliveMsecs, 2000);
Expand All @@ -235,6 +231,7 @@ describe('OTLPLogExporter - node with proto over http', () => {
done();
return fakeRequest as any;
});
collectorExporter.export(logs, () => {});
});

it('should successfully send the logs', done => {
Expand Down Expand Up @@ -271,35 +268,35 @@ describe('OTLPLogExporter - node with proto over http', () => {
// Need to stub/spy on the underlying logger as the "diag" instance is global
const spyLoggerError = sinon.stub(diag, 'error');

collectorExporter.export(logs, result => {
assert.strictEqual(result.code, ExportResultCode.SUCCESS);
assert.strictEqual(spyLoggerError.args.length, 0);
done();
});

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

it('should log the error message', done => {
collectorExporter.export(logs, result => {
assert.strictEqual(result.code, ExportResultCode.FAILED);
// @ts-expect-error verify error code
assert.strictEqual(result.error.code, 400);
assert.strictEqual(result.code, ExportResultCode.SUCCESS);
assert.strictEqual(spyLoggerError.args.length, 0);
done();
});
});

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');

return fakeRequest as any;
});

collectorExporter.export(logs, result => {
assert.strictEqual(result.code, ExportResultCode.FAILED);
// @ts-expect-error verify error code
assert.strictEqual(result.error.code, 400);
done();
});
});
});
describe('export - with compression', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,11 @@ import {
OTLPGRPCExporterNodeBase,
validateAndNormalizeUrl,
DEFAULT_COLLECTOR_URL,
TraceSerializer,
} from '@opentelemetry/otlp-grpc-exporter-base';
import {
createExportTraceServiceRequest,
IExportTraceServiceRequest,
IExportTraceServiceResponse,
ProtobufTraceSerializer,
} from '@opentelemetry/otlp-transformer';
import { VERSION } from './version';

Expand Down Expand Up @@ -57,14 +56,10 @@ export class OTLPTraceExporter
signalSpecificMetadata,
'TraceExportService',
'/opentelemetry.proto.collector.trace.v1.TraceService/Export',
TraceSerializer
ProtobufTraceSerializer
);
}

convert(spans: ReadableSpan[]): IExportTraceServiceRequest {
return createExportTraceServiceRequest(spans);
}

getDefaultUrl(config: OTLPGRPCExporterConfigNode) {
return validateAndNormalizeUrl(this.getUrlFromConfig(config));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ import {

const traceServiceProtoPath =
'opentelemetry/proto/collector/trace/v1/trace_service.proto';
const includeDirs = [
path.resolve(__dirname, '../../otlp-grpc-exporter-base/protos'),
];
const includeDirs = [path.resolve(__dirname, '../../otlp-transformer/protos')];

const httpAddr = 'https://localhost:1501';
const udsAddr = 'unix:///tmp/otlp-traces.sock';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ import {
appendRootPathToUrlIfNeeded,
} from '@opentelemetry/otlp-exporter-base';
import {
createExportTraceServiceRequest,
IExportTraceServiceRequest,
IExportTraceServiceResponse,
} from '@opentelemetry/otlp-transformer';
import { VERSION } from '../../version';
import { JsonTraceSerializer } from '@opentelemetry/otlp-transformer';

const DEFAULT_COLLECTOR_RESOURCE_PATH = 'v1/traces';
const DEFAULT_COLLECTOR_URL = `http://localhost:4318/${DEFAULT_COLLECTOR_RESOURCE_PATH}`;
Expand All @@ -41,11 +42,15 @@ const USER_AGENT = {
* Collector Trace Exporter for Node
*/
export class OTLPTraceExporter
extends OTLPExporterNodeBase<ReadableSpan, IExportTraceServiceRequest>
extends OTLPExporterNodeBase<
ReadableSpan,
IExportTraceServiceRequest,
IExportTraceServiceResponse
>
implements SpanExporter
{
constructor(config: OTLPExporterNodeConfigBase = {}) {
super(config);
super(config, JsonTraceSerializer, 'application/json');
this.headers = {
...this.headers,
...USER_AGENT,
Expand All @@ -56,13 +61,6 @@ export class OTLPTraceExporter
};
}

convert(spans: ReadableSpan[]): IExportTraceServiceRequest {
return createExportTraceServiceRequest(spans, {
useHex: true,
useLongBits: false,
});
}

getDefaultUrl(config: OTLPExporterNodeConfigBase): string {
return typeof config.url === 'string'
? config.url
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,14 @@ import {
OTLPExporterNodeConfigBase,
appendResourcePathToUrl,
appendRootPathToUrlIfNeeded,
OTLPExporterNodeBase,
parseHeaders,
} from '@opentelemetry/otlp-exporter-base';
import { ServiceClientType } from '@opentelemetry/otlp-proto-exporter-base';
import {
OTLPProtoExporterNodeBase,
ServiceClientType,
} from '@opentelemetry/otlp-proto-exporter-base';
import {
createExportTraceServiceRequest,
IExportTraceServiceRequest,
IExportTraceServiceResponse,
ProtobufTraceSerializer,
} from '@opentelemetry/otlp-transformer';
import { VERSION } from '../../version';

Expand All @@ -42,11 +41,15 @@ const USER_AGENT = {
* Collector Trace Exporter for Node with protobuf
*/
export class OTLPTraceExporter
extends OTLPProtoExporterNodeBase<ReadableSpan, IExportTraceServiceRequest>
extends OTLPExporterNodeBase<
ReadableSpan,
IExportTraceServiceRequest,
IExportTraceServiceResponse
>
implements SpanExporter
{
constructor(config: OTLPExporterNodeConfigBase = {}) {
super(config);
super(config, ProtobufTraceSerializer, 'application/x-protobuf');
this.headers = {
...this.headers,
...USER_AGENT,
Expand All @@ -57,10 +60,6 @@ export class OTLPTraceExporter
};
}

convert(spans: ReadableSpan[]): IExportTraceServiceRequest {
return createExportTraceServiceRequest(spans);
}

getDefaultUrl(config: OTLPExporterNodeConfigBase) {
return typeof config.url === 'string'
? config.url
Expand Down
Loading