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

refactor(otlp-exporter-base): don't create blob before sending xhr #5193

Merged
merged 6 commits into from
Nov 25, 2024
Merged

refactor(otlp-exporter-base): don't create blob before sending xhr #5193

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a241bf6
fix(otlp-exporter-base): don't create blob before sending xhr
pichlermarc Nov 22, 2024
57c1dc9
fixup! fix(otlp-exporter-base): don't create blob before sending xhr
pichlermarc Nov 22, 2024
079511f
chore: add changelog entry
pichlermarc Nov 22, 2024
82a7f24
Merge branch 'main' into fix/xhr-blob
pichlermarc Nov 22, 2024
d8eee42
chore: update changelog
pichlermarc Nov 25, 2024
3ec05b0
chore: add github handle to changelog
pichlermarc Nov 25, 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
3 changes: 3 additions & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ All notable changes to experimental packages in this project will be documented

### :bug: (Bug Fix)

* fix(otlp-exporter-base): don't create blob before sending xhr [#5193](https://github.com/open-telemetry/opentelemetry-js/pull/5193)
* improves compatibility with some unsupported runtimes
pichlermarc marked this conversation as resolved.
Show resolved Hide resolved

### :books: (Refine Doc)

### :house: (Internal)
Expand Down
23 changes: 13 additions & 10 deletions experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,9 @@ describe('OTLPTraceExporter - web', () => {
assert.strictEqual(request.method, 'POST');
assert.strictEqual(request.url, 'http://foo.bar.com');

const body = request.requestBody as Blob;
const decoder = new TextDecoder();
const body = request.requestBody as Uint8Array;
const json = JSON.parse(
decoder.decode(await body.arrayBuffer())
new TextDecoder().decode(body)
) as IExportTraceServiceRequest;
const span1 = json.resourceSpans?.[0].scopeSpans?.[0].spans?.[0];

Expand Down Expand Up @@ -340,14 +339,18 @@ describe('OTLPTraceExporter - web', () => {
collectorTraceExporter.export(spans, () => {});

queueMicrotask(() => {
const [{ requestHeaders }] = server.requests;

ensureHeadersContain(requestHeaders, customHeaders);
assert.strictEqual(stubBeacon.callCount, 0);
assert.strictEqual(stubOpen.callCount, 0);
try {
const [{ requestHeaders: requestHeaders }] = server.requests;

clock.restore();
done();
ensureHeadersContain(requestHeaders, customHeaders);
assert.strictEqual(stubBeacon.callCount, 0);
assert.strictEqual(stubOpen.callCount, 0);
done();
} catch (e) {
done(e);
} finally {
clock.restore();
}
});
});
it('should log the timeout request error message', done => {
Expand Down
137 changes: 72 additions & 65 deletions ...ges/opentelemetry-exporter-metrics-otlp-http/test/browser/CollectorMetricExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,77 +240,84 @@ describe('OTLPMetricExporter - web', () => {
collectorExporter.export(metrics, () => {});

queueMicrotask(async () => {
const request = server.requests[0];
assert.strictEqual(request.method, 'POST');
assert.strictEqual(request.url, 'http://foo.bar.com');

const body = request.requestBody;
const decoder = new TextDecoder();
const json = JSON.parse(
decoder.decode(await body.arrayBuffer())
) as IExportMetricsServiceRequest;
// The order of the metrics is not guaranteed.
const counterIndex = metrics.scopeMetrics[0].metrics.findIndex(
it => it.descriptor.name === 'int-counter'
);
const observableIndex = metrics.scopeMetrics[0].metrics.findIndex(
it => it.descriptor.name === 'double-observable-gauge2'
);
const histogramIndex = metrics.scopeMetrics[0].metrics.findIndex(
it => it.descriptor.name === 'int-histogram'
);

const metric1 =
json.resourceMetrics[0].scopeMetrics[0].metrics[counterIndex];
const metric2 =
json.resourceMetrics[0].scopeMetrics[0].metrics[observableIndex];
const metric3 =
json.resourceMetrics[0].scopeMetrics[0].metrics[histogramIndex];
try {
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: I wrapped this in a try/catch to ensure done is called - otherwise if the tests fails it will just say that the test timed out.

const request = server.requests[0];
assert.strictEqual(request.method, 'POST');
assert.strictEqual(request.url, 'http://foo.bar.com');

const body = request.requestBody;
const json = JSON.parse(
new TextDecoder().decode(body)
) as IExportMetricsServiceRequest;
Comment on lines +249 to +251
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: this is the only thing that really changed here, since we're not passing a Blob this test needed to be adapted.

// The order of the metrics is not guaranteed.
const counterIndex = metrics.scopeMetrics[0].metrics.findIndex(
it => it.descriptor.name === 'int-counter'
);
const observableIndex = metrics.scopeMetrics[0].metrics.findIndex(
it => it.descriptor.name === 'double-observable-gauge2'
);
const histogramIndex = metrics.scopeMetrics[0].metrics.findIndex(
it => it.descriptor.name === 'int-histogram'
);

assert.ok(typeof metric1 !== 'undefined', "metric doesn't exist");
ensureCounterIsCorrect(
metric1,
metrics.scopeMetrics[0].metrics[counterIndex].dataPoints[0].endTime,
metrics.scopeMetrics[0].metrics[counterIndex].dataPoints[0]
.startTime
);
const metric1 =
json.resourceMetrics[0].scopeMetrics[0].metrics[counterIndex];
const metric2 =
json.resourceMetrics[0].scopeMetrics[0].metrics[observableIndex];
const metric3 =
json.resourceMetrics[0].scopeMetrics[0].metrics[histogramIndex];

assert.ok(typeof metric1 !== 'undefined', "metric doesn't exist");
ensureCounterIsCorrect(
metric1,
metrics.scopeMetrics[0].metrics[counterIndex].dataPoints[0]
.endTime,
metrics.scopeMetrics[0].metrics[counterIndex].dataPoints[0]
.startTime
);

assert.ok(
typeof metric2 !== 'undefined',
"second metric doesn't exist"
);
ensureObservableGaugeIsCorrect(
metric2,
metrics.scopeMetrics[0].metrics[observableIndex].dataPoints[0]
.endTime,
metrics.scopeMetrics[0].metrics[observableIndex].dataPoints[0]
.startTime,
6,
'double-observable-gauge2'
);
assert.ok(
typeof metric2 !== 'undefined',
"second metric doesn't exist"
);
ensureObservableGaugeIsCorrect(
metric2,
metrics.scopeMetrics[0].metrics[observableIndex].dataPoints[0]
.endTime,
metrics.scopeMetrics[0].metrics[observableIndex].dataPoints[0]
.startTime,
6,
'double-observable-gauge2'
);

assert.ok(
typeof metric3 !== 'undefined',
"third metric doesn't exist"
);
ensureHistogramIsCorrect(
metric3,
metrics.scopeMetrics[0].metrics[histogramIndex].dataPoints[0]
.endTime,
metrics.scopeMetrics[0].metrics[histogramIndex].dataPoints[0]
.startTime,
[0, 100],
[0, 2, 0]
);
assert.ok(
typeof metric3 !== 'undefined',
"third metric doesn't exist"
);
ensureHistogramIsCorrect(
metric3,
metrics.scopeMetrics[0].metrics[histogramIndex].dataPoints[0]
.endTime,
metrics.scopeMetrics[0].metrics[histogramIndex].dataPoints[0]
.startTime,
[0, 100],
[0, 2, 0]
);

const resource = json.resourceMetrics[0].resource;
assert.ok(typeof resource !== 'undefined', "resource doesn't exist");
ensureWebResourceIsCorrect(resource);
const resource = json.resourceMetrics[0].resource;
assert.ok(
typeof resource !== 'undefined',
"resource doesn't exist"
);
ensureWebResourceIsCorrect(resource);

assert.strictEqual(stubBeacon.callCount, 0);
ensureExportMetricsServiceRequestIsSet(json);
assert.strictEqual(stubBeacon.callCount, 0);
ensureExportMetricsServiceRequestIsSet(json);

done();
done();
} catch (e) {
done(e);
}
});
});

Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/otlp-exporter-base/src/transport/xhr-transport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class XhrTransport implements IExporterTransport {
});
};

xhr.send(new Blob([data], { type: headers['Content-Type'] }));
xhr.send(data);
});
}

Expand Down
5 changes: 1 addition & 4 deletions experimental/packages/otlp-exporter-base/test/browser/xhr-transport.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,7 @@ describe('XhrTransport', function () {
undefined
);
assert.strictEqual(request.url, testTransportParameters.url);
assert.strictEqual(
(request.requestBody as unknown as Blob).type,
'application/json'
);
assert.strictEqual(request.requestBody, testPayload);
ensureHeadersContain(request.requestHeaders, {
foo: 'foo-value',
bar: 'bar-value',
Expand Down