Skip to content

Commit

Permalink
chore(http): remove x-opentelemetry-outgoing-request header #1547
Browse files Browse the repository at this point in the history
  • Loading branch information
vmarchaud committed Sep 19, 2020
1 parent 3309ea4 commit 73ad589
Show file tree
Hide file tree
Showing 14 changed files with 2 additions and 192 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ export abstract class CollectorExporterNodeBase<
ExportItem,
ServiceRequest
> {
DEFAULT_HEADERS: Record<string, string> = {
[collectorTypes.OT_REQUEST_HEADER]: '1',
};
DEFAULT_HEADERS: Record<string, string> = {};
headers: Record<string, string>;
constructor(config: CollectorExporterConfigBase = {}) {
super(config);
Expand Down
3 changes: 0 additions & 3 deletions packages/opentelemetry-exporter-collector/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@
import { SpanKind, Logger, Attributes } from '@opentelemetry/api';
import * as api from '@opentelemetry/api';

// header to prevent instrumentation on request
export const OT_REQUEST_HEADER = 'x-opentelemetry-outgoing-request';

/* eslint-disable @typescript-eslint/no-namespace */
export namespace opentelemetryProto {
export namespace collector {
Expand Down
2 changes: 0 additions & 2 deletions packages/opentelemetry-exporter-jaeger/src/jaeger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import { ReadableSpan, SpanExporter } from '@opentelemetry/tracing';
import { Socket } from 'dgram';
import { spanToThrift } from './transform';
import * as jaegerTypes from './types';
import { OT_REQUEST_HEADER } from './utils';

/**
* Format and sends span information to Jaeger Exporter.
Expand Down Expand Up @@ -54,7 +53,6 @@ export class JaegerExporter implements SpanExporter {
localConfig.host = localConfig.host || process.env.JAEGER_AGENT_HOST;
if (localConfig.endpoint) {
this._sender = new jaegerTypes.HTTPSender(localConfig);
this._sender._httpOptions.headers[OT_REQUEST_HEADER] = 1;
} else {
this._sender = localConfig.endpoint = new jaegerTypes.UDPSender(
localConfig
Expand Down
17 changes: 0 additions & 17 deletions packages/opentelemetry-exporter-jaeger/src/utils.ts

This file was deleted.

8 changes: 1 addition & 7 deletions packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { ThriftProcess } from '../src/types';
import { ReadableSpan } from '@opentelemetry/tracing';
import { TraceFlags } from '@opentelemetry/api';
import { Resource } from '@opentelemetry/resources';
import { OT_REQUEST_HEADER } from '../src/utils';
import * as nock from 'nock';

describe('JaegerExporter', () => {
Expand Down Expand Up @@ -156,12 +155,11 @@ describe('JaegerExporter', () => {
});
});

it('should use httpSender if config.endpoint is setten and set x-opentelemetry-outgoing-request header', done => {
it('should use httpSender if config.endpoint is setten', done => {
const mockedEndpoint = 'http://testendpoint';
nock(mockedEndpoint)
.post('/')
.reply(function () {
assert.strictEqual(this.req.headers[OT_REQUEST_HEADER], 1);
assert.strictEqual(
this.req.headers['content-type'],
'application/x-thrift'
Expand All @@ -174,10 +172,6 @@ describe('JaegerExporter', () => {
endpoint: mockedEndpoint,
});
assert.strictEqual(exporter['_sender'].constructor.name, 'HTTPSender');
assert.strictEqual(
exporter['_sender']._httpOptions.headers[OT_REQUEST_HEADER],
1
);
const spanContext = {
traceId: 'd4cda95b652f4a1592b449d5929fda1b',
spanId: '6e0c63257de34c92',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import * as api from '@opentelemetry/api';
import { ExportResult } from '@opentelemetry/core';
import * as zipkinTypes from '../../types';
import { OT_REQUEST_HEADER } from '../../utils';

/**
* Prepares send function that will send spans to the remote Zipkin service.
Expand All @@ -33,7 +32,6 @@ export function prepareSend(
xhrHeaders = {
Accept: 'application/json',
'Content-Type': 'application/json',
[OT_REQUEST_HEADER]: '1',
...headers,
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import * as http from 'http';
import * as https from 'https';
import * as url from 'url';
import * as zipkinTypes from '../../types';
import { OT_REQUEST_HEADER } from '../../utils';

/**
* Prepares send function that will send spans to the remote Zipkin service.
Expand All @@ -37,7 +36,6 @@ export function prepareSend(
method: 'POST',
headers: {
'Content-Type': 'application/json',
[OT_REQUEST_HEADER]: 1,
...headers,
},
},
Expand Down
17 changes: 0 additions & 17 deletions packages/opentelemetry-exporter-zipkin/src/utils.ts

This file was deleted.

21 changes: 0 additions & 21 deletions packages/opentelemetry-exporter-zipkin/test/node/zipkin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import * as api from '@opentelemetry/api';
import { Resource } from '@opentelemetry/resources';
import { ZipkinExporter } from '../../src';
import * as zipkinTypes from '../../src/types';
import { OT_REQUEST_HEADER } from '../../src/utils';
import { TraceFlags } from '@opentelemetry/api';
import { SERVICE_RESOURCE } from '@opentelemetry/resources';

Expand Down Expand Up @@ -254,26 +253,6 @@ describe('Zipkin Exporter - node', () => {
});
});

it(`should send '${OT_REQUEST_HEADER}' header`, () => {
const scope = nock('https://localhost:9411')
.post('/api/v2/spans')
.reply(function (uri, requestBody, cb) {
assert.ok(this.req.headers[OT_REQUEST_HEADER]);
cb(null, [200, 'Ok']);
});

const exporter = new ZipkinExporter({
serviceName: 'my-service',
logger: new NoopLogger(),
url: 'https://localhost:9411/api/v2/spans',
});

exporter.export([getReadableSpan()], (result: ExportResult) => {
scope.done();
assert.strictEqual(result, ExportResult.SUCCESS);
});
});

it('should return FailedNonRetryable with 4xx', () => {
const scope = nock('http://localhost:9411')
.post('/api/v2/spans')
Expand Down
8 changes: 0 additions & 8 deletions packages/opentelemetry-plugin-http/src/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,15 +398,7 @@ export class HttpPlugin extends BasePlugin<Http> {
extraOptions
);

if (utils.isOpenTelemetryRequest(optionsParsed)) {
// clone the headers so delete will not modify the user's object
optionsParsed.headers = Object.assign({}, optionsParsed.headers);
delete optionsParsed.headers[utils.OT_REQUEST_HEADER];
return original.apply(this, [optionsParsed, ...args]);
}

if (
utils.isOpenTelemetryRequest(optionsParsed) ||
utils.isIgnored(
origin + pathname,
plugin._config.ignoreOutgoingUrls,
Expand Down
24 changes: 0 additions & 24 deletions packages/opentelemetry-plugin-http/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,6 @@ import {
SpecialHttpStatusCodeMapping,
} from './types';

/**
* Specific header used by exporters to "mark" outgoing request to avoid creating
* spans for request that export them which would create a infinite loop.
*/
export const OT_REQUEST_HEADER = 'x-opentelemetry-outgoing-request';

export const HTTP_STATUS_SPECIAL_CASES: SpecialHttpStatusCodeMapping = {
401: CanonicalCode.UNAUTHENTICATED,
403: CanonicalCode.PERMISSION_DENIED,
Expand Down Expand Up @@ -301,24 +295,6 @@ export const isValidOptionsType = (options: unknown): boolean => {
return type === 'string' || (type === 'object' && !Array.isArray(options));
};

/**
* Check whether the given request should be ignored
* Use case: Typically, exporter `SpanExporter` can use http module to send spans.
* This will also generate spans (from the http-plugin) that will be sended through the exporter
* and here we have loop.
*
* TODO: Refactor this logic when a solution is found in
* https://github.com/open-telemetry/opentelemetry-specification/issues/530
*
*
* @param {RequestOptions} options
*/
export const isOpenTelemetryRequest = (
options: RequestOptions
): options is { headers: {} } & RequestOptions => {
return !!(options && options.headers && options.headers[OT_REQUEST_HEADER]);
};

/**
* Returns outgoing request attributes scoped to the options passed to the request
* @param {ParsedRequestOptions} requestOptions the same options used to make the request
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import * as nock from 'nock';
import * as path from 'path';
import { HttpPlugin, plugin } from '../../src/http';
import { Http, HttpPluginConfig } from '../../src/types';
import { OT_REQUEST_HEADER } from '../../src/utils';
import { assertSpan } from '../utils/assertSpan';
import { DummyPropagation } from '../utils/DummyPropagation';
import { httpRequest } from '../utils/httpRequest';
Expand Down Expand Up @@ -185,27 +184,6 @@ describe('HttpPlugin', () => {
serverPort
);
});

it(`should not trace requests with '${OT_REQUEST_HEADER}' header`, async () => {
const testPath = '/outgoing/do-not-trace';
doNock(hostname, testPath, 200, 'Ok');

const options = {
host: hostname,
path: testPath,
headers: { [OT_REQUEST_HEADER]: 1 },
};

const result = await httpRequest.get(options);
assert(
result.reqHeaders[OT_REQUEST_HEADER] === undefined,
'custom header should be stripped'
);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(result.data, 'Ok');
assert.strictEqual(spans.length, 0);
assert.strictEqual(options.headers[OT_REQUEST_HEADER], 1);
});
});
describe('with good plugin options', () => {
beforeEach(() => {
Expand Down Expand Up @@ -303,26 +281,6 @@ describe('HttpPlugin', () => {
});
});

it(`should not trace requests with '${OT_REQUEST_HEADER}' header`, async () => {
const testPath = '/outgoing/do-not-trace';
doNock(hostname, testPath, 200, 'Ok');

const options = {
host: hostname,
path: testPath,
headers: { [OT_REQUEST_HEADER]: 1 },
};

const result = await httpRequest.get(options);
assert(
result.reqHeaders[OT_REQUEST_HEADER] === undefined,
'custom header should be stripped'
);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(result.data, 'Ok');
assert.strictEqual(spans.length, 0);
});

const httpErrorCodes = [
400,
401,
Expand Down
27 changes: 0 additions & 27 deletions packages/opentelemetry-plugin-http/test/functionals/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,33 +271,6 @@ describe('Utility', () => {
}
});
});
describe('isOpenTelemetryRequest()', () => {
[
{},
{ headers: {} },
url.parse('http://url.com'),
{ headers: { [utils.OT_REQUEST_HEADER]: 0 } },
{ headers: { [utils.OT_REQUEST_HEADER]: false } },
].forEach(options => {
it(`should return false with the following value: ${JSON.stringify(
options
)}`, () => {
/* tslint:disable-next-line:no-any */
assert.strictEqual(utils.isOpenTelemetryRequest(options as any), false);
});
});
for (const options of [
{ headers: { [utils.OT_REQUEST_HEADER]: 1 } },
{ headers: { [utils.OT_REQUEST_HEADER]: true } },
]) {
it(`should return true with the following value: ${JSON.stringify(
options
)}`, () => {
/* tslint:disable-next-line:no-any */
assert.strictEqual(utils.isOpenTelemetryRequest(options as any), true);
});
}
});

describe('isValidOptionsType()', () => {
['', false, true, 1, 0, []].forEach(options => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import { NodeTracerProvider } from '@opentelemetry/node';
import {
Http,
HttpPluginConfig,
OT_REQUEST_HEADER,
} from '@opentelemetry/plugin-http';
import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
import { ContextManager } from '@opentelemetry/context-base';
Expand Down Expand Up @@ -186,22 +185,6 @@ describe('HttpsPlugin', () => {
serverPort
);
});

it(`should not trace requests with '${OT_REQUEST_HEADER}' header`, async () => {
const testPath = '/outgoing/do-not-trace';
doNock(hostname, testPath, 200, 'Ok');

const options = {
host: hostname,
path: testPath,
headers: { [OT_REQUEST_HEADER]: 1 },
};

const result = await httpsRequest.get(options);
const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(result.data, 'Ok');
assert.strictEqual(spans.length, 0);
});
});
describe('with good plugin options', () => {
beforeEach(() => {
Expand Down

0 comments on commit 73ad589

Please sign in to comment.