Skip to content

Commit

Permalink
fix(exporter-collector): set content-encoding header only when compre…
Browse files Browse the repository at this point in the history 
…ssing
  • Loading branch information
@alisabzevari
alisabzevari committed Jul 12, 2021
1 parent 6bceae0 commit 8805cea
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 1 deletion.
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector/src/platform/node/util.ts
View file
Expand Up @@ -51,7 +51,6 @@ export function sendWithHttp<ExportItem, ServiceRequest>(
headers: {
'Content-Length': Buffer.byteLength(data),
'Content-Type': contentType,
'Content-Encoding': 'gzip',
...collector.headers,
},
agent: collector.agent,
Expand Down Expand Up @@ -84,6 +83,7 @@ export function sendWithHttp<ExportItem, ServiceRequest>(

switch (collector.compression) {
case CompressionAlgorithm.GZIP: {
req.setHeader('Content-Encoding', 'gzip');

This comment has been minimized.

Sign in to view

Copy link
@obecny

obecny Jul 12, 2021

Member

this might be tricky. User maybe be passing such header already with more options deflate, gzip for example and then it will try to override this header again. The safest approach should be to set this first and then override with collector headers. I'm talking about this

  const headers: OutgoingHttpHeaders = {
    'Content-Length': Buffer.byteLength(data),
    'Content-Type': contentType,
  };
  if (collector.compression) {
    headers['Content-Encoding'] = 'gzip';
  }
  const options: http.RequestOptions | https.RequestOptions = {
    hostname: parsedUrl.hostname,
    port: parsedUrl.port,
    path: parsedUrl.pathname,
    method: 'POST',
    headers: {
      ...headers,
      ...collector.headers,
    },
    agent: collector.agent,
  };

This comment has been minimized.

Sign in to view

Copy link
@alisabzevari

alisabzevari Jul 13, 2021 •
edited

Author Contributor

I thought the compression concerns should be hidden from the user in this function. What is the valid use-case for the user to set the Content-Encoding?
const dataStream = readableFromBuffer(data);
dataStream.on('error', onError)
.pipe(gzip).on('error', onError)
Expand Down
15 changes: 15 additions & 0 deletions packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts
View file
Expand Up @@ -44,6 +44,7 @@ describe('CollectorTraceExporter - node with json over http', () => {
let collectorExporter: CollectorTraceExporter;
let collectorExporterConfig: CollectorExporterNodeConfigBase;
let stubRequest: sinon.SinonStub;
let spySetHeader: sinon.SinonSpy;
let spans: ReadableSpan[];

afterEach(() => {
Expand Down Expand Up @@ -148,6 +149,17 @@ describe('CollectorTraceExporter - node with json over http', () => {
});
});

it('should not have Content-Encoding header', done => {
collectorExporter.export(spans, () => { });

setTimeout(() => {
const args = stubRequest.args[0];
const options = args[0];
assert.strictEqual(options.headers['Content-Encoding'], undefined);
done();
});
});

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

Expand Down Expand Up @@ -251,6 +263,8 @@ describe('CollectorTraceExporter - node with json over http', () => {
describe('export - with compression', () => {
beforeEach(() => {
stubRequest = sinon.stub(http, 'request').returns(fakeRequest as any);
spySetHeader = sinon.spy();
(fakeRequest as any).setHeader = spySetHeader;
collectorExporterConfig = {
headers: {
foo: 'bar',
Expand Down Expand Up @@ -285,6 +299,7 @@ describe('CollectorTraceExporter - node with json over http', () => {
}

ensureExportTraceServiceRequestIsSet(json);
assert.ok(spySetHeader.calledWith('Content-Encoding', 'gzip'));

done();
});
Expand Down

0 comments on commit 8805cea

Please sign in to comment.