Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

HTTP Propagation: set Correlation-Context header #567

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

HTTP Propagation: set Correlation-Context header #567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b9283e
set Correlation-Context header
mayurkale22 May 30, 2019
4bf8ba1
Rename TagContext -> TagMap
mayurkale22 May 31, 2019
0887b0d
Fix build
mayurkale22 Jun 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ All notable changes to this project will be documented in this file.

## Unreleased

- Add support for HTTP tags propagation.

## 0.0.14 - 2019-06-04
- Exporter/Stats/Stackdriver: Add support for exemplar
Expand Down
36 changes: 34 additions & 2 deletions packages/opencensus-instrumentation-http/src/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,12 @@ import {
TagMap,
TagTtl,
TraceOptions,
serializeTextFormat,
deserializeTextFormat,
} from '@opencensus/core';
import {
ClientRequest,
IncomingHttpHeaders,
IncomingMessage,
request,
RequestOptions,
Expand All @@ -37,6 +40,7 @@ import {
import * as semver from 'semver';
import * as shimmer from 'shimmer';
import * as url from 'url';

import * as stats from './http-stats';
import { HttpPluginConfig, IgnoreMatcher } from './types';

Expand All @@ -56,6 +60,8 @@ const UNLIMITED_PROPAGATION_MD = {
};

const TAG_VALUE_MAX_LENGTH = 255;
/** A correlation context header under which TagMap is stored as a text value */
export const CORRELATION_CONTEXT = 'Correlation-Context';

/** Http instrumentation plugin for Opencensus */
export class HttpPlugin extends BasePlugin {
Expand Down Expand Up @@ -261,7 +267,7 @@ export class HttpPlugin extends BasePlugin {
const host = headers.host || 'localhost';
const userAgent = (headers['user-agent'] ||
headers['User-Agent']) as string;
const tags = new TagMap();
const tags = HttpPlugin.getTagMap(headers) || new TagMap();

rootSpan.addAttribute(
HttpPlugin.ATTRIBUTE_HTTP_HOST,
Expand Down Expand Up @@ -483,7 +489,19 @@ export class HttpPlugin extends BasePlugin {
? headers['user-agent'] || headers['User-Agent']
: null;

const tags = new TagMap();
// record stats: new RPCs on client-side inherit the tag context from
// the current Context.
const tags = plugin.stats
? plugin.stats.getCurrentTagContext()
: new TagMap();
if (tags.tags.size > 0) {
if (plugin.hasExpectHeader(options) && options.headers) {
options.headers[CORRELATION_CONTEXT] = serializeTextFormat(tags);
} else {
request.setHeader(CORRELATION_CONTEXT, serializeTextFormat(tags));
}
}

tags.set(stats.HTTP_CLIENT_METHOD, { value: method });

const host = options.hostname || options.host || 'localhost';
Expand Down Expand Up @@ -602,6 +620,20 @@ export class HttpPlugin extends BasePlugin {
} catch (ignore) {}
}

/**
* Returns a TagMap on incoming HTTP header if it exists and is well-formed,
* or null otherwise.
* @param headers The incoming HTTP header object from which TagMap should be
* retrieved.
*/
static getTagMap(headers: IncomingHttpHeaders): TagMap | null {
const contextValue = (headers[CORRELATION_CONTEXT.toLocaleLowerCase()] ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Why toLocaleLowerCase?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK Node core automatically converts all HTTP headers to lowercase. Without much frills, I easily found the location where outgoing HTTP headers are converted to lowercase.

@draffensperger correct me if this statement is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would you think about just making the constant itself be in lower case so you don't need to convert it here along with a comment that Node converts headers to lower case?

I'm not super familiar with how headers in Node are handled. Could this be related? nodejs/node-v0.x-archive#1954
How are headers for trace context handled? Do we assume those are lowercase?

Copy link
Member Author

Choose a reason for hiding this comment

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

TraceContext headers are defined in lower case only. As per official documentation -> In order to increase interoperability across multiple protocols and encourage successful integration by default it is recommended to keep the header name lower case. Platforms and libraries MUST expect header name in any casing and SHOULD send header name in lower case.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I guess I'm a little confused by the code then, to me it looks like you are looking for the header key in lower case rather than converting all the incoming headers to lower case to compare them?

Or am I missing something here?

Copy link
Contributor

Choose a reason for hiding this comment

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

TraceContext headers are defined in lower case only.

That's for TraceContext. I don't think it's the case for Correlation-Context, per https://github.com/w3c/correlation-context/blob/master/correlation_context/HTTP_HEADER_FORMAT.md#examples-of-http-headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Phrased a different way, how do you know that headers: IncomingHttpHeaders here will always have its values in lower case? Could they ever be in upper-case, and if so, should you just convert them to lower case to make sure before comparing them to a lower-cased header?

Copy link
Member Author

Choose a reason for hiding this comment

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

how do you know that headers: IncomingHttpHeaders here will always have its values in lower case? Could they ever be in upper-case?

Not 100% sure on this, perhaps that's why we use both the variant of user-agent key. I think it is safe bet to compare both the variant until we don;t know the complete picture.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Maybe you can just making both the incoming header and the constant be lower case and compare those?

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 going to approve this, and if this comes up as a bug in the future, it should be fixed.

headers[CORRELATION_CONTEXT]) as string;
// Entry doesn't exist.
if (!contextValue) return null;
return deserializeTextFormat(contextValue);
}

/**
* Returns whether the Expect header is on the given options object.
* @param options Options for http.request.
Expand Down
153 changes: 150 additions & 3 deletions packages/opencensus-instrumentation-http/test/test-http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import * as http from 'http';
import * as nock from 'nock';
import * as shimmer from 'shimmer';
import * as url from 'url';

import { HttpPlugin, plugin } from '../src/';
import * as stats from '../src/http-stats';

Expand Down Expand Up @@ -183,11 +182,19 @@ function assertCustomAttribute(
function assertClientStats(
testExporter: TestExporter,
httpStatusCode: number,
httpMethod: string
httpMethod: string,
tagCtx?: TagMap
) {
const tags = new TagMap();
tags.set(stats.HTTP_CLIENT_METHOD, { value: httpMethod });
tags.set(stats.HTTP_CLIENT_STATUS, { value: `${httpStatusCode}` });

if (tagCtx) {
tagCtx.tags.forEach((tagValue: TagValue, tagKey: TagKey) => {
tags.set(tagKey, tagValue);
});
}

assert.strictEqual(testExporter.registeredViews.length, 8);
assert.strictEqual(testExporter.recordedMeasurements.length, 1);
assert.strictEqual(
Expand All @@ -202,12 +209,20 @@ function assertServerStats(
testExporter: TestExporter,
httpStatusCode: number,
httpMethod: string,
path: string
path: string,
tagCtx?: TagMap
) {
const tags = new TagMap();
tags.set(stats.HTTP_SERVER_METHOD, { value: httpMethod });
tags.set(stats.HTTP_SERVER_STATUS, { value: `${httpStatusCode}` });
tags.set(stats.HTTP_SERVER_ROUTE, { value: path });

if (tagCtx) {
tagCtx.tags.forEach((tagValue: TagValue, tagKey: TagKey) => {
tags.set(tagKey, tagValue);
});
}

assert.strictEqual(testExporter.registeredViews.length, 8);
assert.strictEqual(testExporter.recordedMeasurements.length, 1);
assert.strictEqual(
Expand Down Expand Up @@ -357,6 +372,60 @@ describe('HttpPlugin', () => {
});
});

it('should create a child span for GET requests with tag context', () => {
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 sure if I understand these tests - why test spans? Same for other tests.

const testPath = '/outgoing/rootSpan/childs/1';
doNock(urlHost, testPath, 200, 'Ok');
const tags = new TagMap();
tags.set({ name: 'testKey1' }, { value: 'value1' });
tags.set({ name: 'testKey2' }, { value: 'value2' });
return globalStats.withTagContext(tags, async () => {
return tracer.startRootSpan(
{ name: 'TestRootSpan' },
async (root: Span) => {
await httpRequest.get(`${urlHost}${testPath}`).then(result => {
assert.ok(root.name.indexOf('TestRootSpan') >= 0);
assert.strictEqual(root.spans.length, 1);
const [span] = root.spans;
assert.ok(span.name.indexOf(testPath) >= 0);
assert.strictEqual(root.traceId, span.traceId);
assertSpanAttributes(span, 200, 'GET', hostName, testPath);
assert.strictEqual(span.messageEvents.length, 1);
const [messageEvent] = span.messageEvents;
assert.strictEqual(messageEvent.type, MessageEventType.SENT);
assert.strictEqual(messageEvent.id, 1);
assertClientStats(testExporter, 200, 'GET', tags);
});
}
);
});
});

it('should create a child span for GET requests with empty tag context', () => {
const testPath = '/outgoing/rootSpan/childs/1';
doNock(urlHost, testPath, 200, 'Ok');
const tags = new TagMap();
return globalStats.withTagContext(tags, async () => {
return tracer.startRootSpan(
{ name: 'TestRootSpan' },
async (root: Span) => {
await httpRequest.get(`${urlHost}${testPath}`).then(result => {
assert.ok(root.name.indexOf('TestRootSpan') >= 0);
assert.strictEqual(root.spans.length, 1);
const [span] = root.spans;
assert.ok(span.name.indexOf(testPath) >= 0);
assert.strictEqual(root.traceId, span.traceId);
assertSpanAttributes(span, 200, 'GET', hostName, testPath);
assert.strictEqual(span.messageEvents.length, 1);
const [messageEvent] = span.messageEvents;
assert.strictEqual(messageEvent.type, MessageEventType.SENT);
assert.strictEqual(messageEvent.id, 1);
assertClientStats(testExporter, 200, 'GET');
});
}
);
});
});

for (let i = 0; i < httpErrorCodes.length; i++) {
it(`should test a child spans for GET requests with http error ${
httpErrorCodes[i]
Expand Down Expand Up @@ -529,6 +598,84 @@ describe('HttpPlugin', () => {
});
});

it('should handle incoming requests with long request url path', async () => {
const testPath = '/test&code=' + 'a'.repeat(300);
const options = {
host: 'localhost',
path: testPath,
port: serverPort,
headers: { 'User-Agent': 'Android' },
};
shimmer.unwrap(http, 'get');
shimmer.unwrap(http, 'request');
nock.enableNetConnect();

assert.strictEqual(spanVerifier.endedSpans.length, 0);

await httpRequest.get(options).then(result => {
assert.strictEqual(spanVerifier.endedSpans.length, 1);
assert.ok(spanVerifier.endedSpans[0].name.indexOf(testPath) >= 0);
const [span] = spanVerifier.endedSpans;
assertSpanAttributes(
span,
200,
'GET',
'localhost',
testPath,
'Android'
);
assertServerStats(
testExporter,
200,
'GET',
'/test&code=' + 'a'.repeat(244)
);
});
});
it('should create a root span for incoming requests with Correlation Context header', async () => {
const testPath = '/incoming/rootSpan/';
const options = {
host: 'localhost',
path: testPath,
port: serverPort,
headers: {
'User-Agent': 'Android',
'Correlation-Context': 'k1=v1,k2=v2',
},
};

const expectedTagsFromHeaders = new TagMap();
expectedTagsFromHeaders.set({ name: 'k1' }, { value: 'v1' });
expectedTagsFromHeaders.set({ name: 'k2' }, { value: 'v2' });

shimmer.unwrap(http, 'get');
shimmer.unwrap(http, 'request');
nock.enableNetConnect();

assert.strictEqual(spanVerifier.endedSpans.length, 0);

await httpRequest.get(options).then(result => {
assert.ok(spanVerifier.endedSpans[0].name.indexOf(testPath) >= 0);
assert.strictEqual(spanVerifier.endedSpans.length, 1);
const [span] = spanVerifier.endedSpans;
assertSpanAttributes(
span,
200,
'GET',
'localhost',
testPath,
'Android'
);
assertServerStats(
testExporter,
200,
'GET',
testPath,
expectedTagsFromHeaders
);
});
});

it('should handle incoming requests with long request url path', async () => {
const testPath = '/test&code=' + 'a'.repeat(300);
const options = {
Expand Down