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

Separate context propagation (OTEP 66) #769

Merged
merged 31 commits into from
Feb 18, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
90e23f0
feat: separate context propagation
dyladan Feb 6, 2020
5a03016
chore: fix api dependencies on builds
dyladan Feb 6, 2020
04fb334
chore: use bootstrap for doc deps
dyladan Feb 6, 2020
3391207
chore: allow builds as root in docs
dyladan Feb 6, 2020
8467706
chore: bootstrap docs after checking code
dyladan Feb 6, 2020
7076553
chore: allow unsafe builds in docs
dyladan Feb 6, 2020
1318000
chore: fix api test build
dyladan Feb 6, 2020
9415269
Merge branch 'master' into context
dyladan Feb 6, 2020
5533881
chore: lint
dyladan Feb 6, 2020
02605b2
chore: bust cache
dyladan Feb 6, 2020
7961494
chore: revert unnecessary changes from api package
dyladan Feb 6, 2020
02e07cb
chore: revert carrier so it can be a separate pr
dyladan Feb 6, 2020
fb0d170
chore: typo
dyladan Feb 6, 2020
d433528
chore: avoid extra allocation on context set value
dyladan Feb 6, 2020
38cda1d
chore: lint
dyladan Feb 6, 2020
d4caaba
chore: do not extend context
dyladan Feb 7, 2020
d89fcc6
chore: remove extra allocation
dyladan Feb 7, 2020
68234f7
chore: review comments
dyladan Feb 7, 2020
216cac3
Merge remote-tracking branch 'origin/master' into context
dyladan Feb 7, 2020
4d05383
Merge branch 'master' into context
dyladan Feb 11, 2020
2987a59
chore: use variables for context keys
dyladan Feb 12, 2020
5ad4383
chore: use unique symbols to identify context values
dyladan Feb 12, 2020
d5ef1d8
chore: use TODO context reference
dyladan Feb 12, 2020
e750d4b
chore: remove root context from dummy propagator
dyladan Feb 12, 2020
adff03c
chore: lint
dyladan Feb 12, 2020
d33f9c0
Merge remote-tracking branch 'origin/master' into context
dyladan Feb 12, 2020
8e433f1
chore: rename getKey to createKey
dyladan Feb 12, 2020
282a5e1
Merge branch 'master' into context
dyladan Feb 18, 2020
afaf91f
Merge branch 'master' into context
dyladan Feb 18, 2020
e90d731
Merge branch 'master' into context
dyladan Feb 18, 2020
4b356ba
Merge branch 'master' into context
dyladan Feb 18, 2020
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
5 changes: 5 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ jobs:
lint_&_docs:
docker:
- image: node:12
environment:
dyladan marked this conversation as resolved.
Show resolved Hide resolved
NPM_CONFIG_UNSAFE_PERM: true
steps:
- checkout
- run:
Expand All @@ -171,6 +173,9 @@ jobs:
- run:
name: Check code style and linting
command: npm run check
- run:
name: Install API dependencies
command: lerna bootstrap --scope @opentelemetry/api --include-filtered-dependencies
- run:
name: Docs tests
command: npm run docs-test
Expand Down
3 changes: 3 additions & 0 deletions packages/opentelemetry-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
"publishConfig": {
"access": "public"
},
"dependencies": {
Copy link
Member Author

Choose a reason for hiding this comment

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

API now depends on scope base because that is where context is. Context is a separate mechanism outside of the API and the SDK, but which is depended on by both.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe Context and the dummy implementation should be moved like it was done with a other parts recently?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be willing to move Context into it's own package. @mayurkale22 wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Still thinking on this, I will also check how other libraries are doing it.

Copy link
Member

Choose a reason for hiding this comment

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

When we started it we decided to move it back so it could be used by other projects. However since it's started to get heavily linked to the OT specs (and not storing generic context) i would be in favor to move it

Copy link
Member Author

Choose a reason for hiding this comment

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

@vmarchaud I'm sorry I don't understand, you would be in favor of context having it's own package or you would be in favor of moving it into core/api/other?

"@opentelemetry/scope-base": "^0.4.0"
},
"devDependencies": {
"@types/mocha": "^5.2.7",
"@types/node": "^12.6.8",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,39 +14,35 @@
* limitations under the License.
*/

import { SpanContext } from '../../trace/span_context';
import { Context } from '@opentelemetry/scope-base';
import { Carrier } from './carrier';

/**
* Injects and extracts a value as text into carriers that travel in-band
* across process boundaries. Encoding is expected to conform to the HTTP
* Injects {@link Context} into and extracts it from carriers that travel
* in-band across process boundaries. Encoding is expected to conform to the HTTP
* Header Field semantics. Values are often encoded as RPC/HTTP request headers.
*
* The carrier of propagated data on both the client (injector) and server
* (extractor) side is usually an http request. Propagation is usually
* implemented via library- specific request interceptors, where the
* client-side injects values and the server-side extracts them.
* (extractor) side is usually an object such as http headers.
*/
export interface HttpTextFormat {
/**
* Injects the given {@link SpanContext} instance to transmit over the wire.
* Injects values from a given {@link Context} into a carrier.
*
* OpenTelemetry defines a common set of format values (BinaryFormat and
* HTTPTextFormat), and each has an expected `carrier` type.
*
* @param spanContext the SpanContext to transmit over the wire.
* @param format the format of the carrier.
* @param carrier the carrier of propagation fields, such as an http request.
* @param context the Context from which to extract values to transmit over the wire.
* @param carrier the carrier of propagation fields, such as http request headers.
*/
inject(spanContext: SpanContext, format: string, carrier: unknown): void;
inject(context: Context, carrier: Carrier): void;

/**
* Returns a {@link SpanContext} instance extracted from `carrier` in the
* given format from upstream.
* Given a {@link Context} and a carrier, extract context values from a carrier and
* return a new context, created from the old context, with the extracted values.
*
* @param format the format of the carrier.
* @param carrier the carrier of propagation fields, such as an http request.
* @returns SpanContext The extracted SpanContext, or null if no such
* SpanContext could be found in carrier.
* @param context the Context from which to extract values to transmit over the wire.
* @param carrier the carrier of propagation fields, such as http request headers.
*/
extract(format: string, carrier: unknown): SpanContext | null;
extract(context: Context, carrier: Carrier): Context;
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,19 @@
* limitations under the License.
*/

import { SpanContext } from '../../trace/span_context';
import { Context } from '@opentelemetry/scope-base';
import { Carrier } from './carrier';
import { HttpTextFormat } from './HttpTextFormat';

/**
* No-op implementations of {@link HttpTextFormat}.
*/
export class NoopHttpTextFormat implements HttpTextFormat {
// By default does nothing
inject(spanContext: SpanContext, format: string, carrier: unknown): void {}
// By default does nothing
extract(format: string, carrier: unknown): SpanContext | null {
return null;
/** Noop inject function does nothing */
inject(context: Context, carrier: Carrier): void {}
/** Noop extract function does nothing and returns the input context */
extract(context: Context, carrier: Carrier): Context {
return context;
}
}

Expand Down
19 changes: 19 additions & 0 deletions packages/opentelemetry-api/src/context/propagation/carrier.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*!
* Copyright 2020, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export type Carrier = {
[key: string]: unknown;
};

This file was deleted.

45 changes: 0 additions & 45 deletions packages/opentelemetry-api/src/distributed_context/EntryValue.ts

This file was deleted.

21 changes: 10 additions & 11 deletions packages/opentelemetry-api/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,33 +17,32 @@
export * from './common/Logger';
export * from './common/Time';
export * from './context/propagation/BinaryFormat';
export * from './context/propagation/carrier';
export * from './context/propagation/HttpTextFormat';
export * from './distributed_context/DistributedContext';
export * from './distributed_context/EntryValue';
export * from './metrics/BoundInstrument';
export * from './metrics/Meter';
export * from './metrics/MeterProvider';
export * from './metrics/Metric';
export * from './metrics/NoopMeter';
export * from './metrics/NoopMeterProvider';
export * from './trace/attributes';
export * from './trace/Event';
export * from './trace/instrumentation/Plugin';
export * from './trace/link';
export * from './trace/NoopSpan';
export * from './trace/NoopTracer';
export * from './trace/NoopTracerProvider';
export * from './trace/Sampler';
export * from './trace/span';
export * from './trace/SpanOptions';
export * from './trace/span_context';
export * from './trace/span_kind';
export * from './trace/span';
export * from './trace/SpanOptions';
export * from './trace/status';
export * from './trace/TimedEvent';
export * from './trace/tracer';
export * from './trace/tracer_provider';
export * from './trace/trace_flags';
export * from './trace/trace_state';
export * from './trace/NoopSpan';
export * from './trace/NoopTracer';
export * from './trace/NoopTracerProvider';
export * from './metrics/NoopMeterProvider';
export * from './metrics/NoopMeter';
export * from './trace/tracer_provider';
export * from './trace/tracer';

import { TraceAPI } from './api/trace';
/** Entrypoint for trace API */
Expand Down
9 changes: 1 addition & 8 deletions packages/opentelemetry-api/src/metrics/BoundInstrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

import { DistributedContext } from '../distributed_context/DistributedContext';
import { SpanContext } from '../trace/span_context';

/** An Instrument for Counter Metric. */
Expand All @@ -40,15 +39,9 @@ export interface BoundMeasure {
/**
* Records the given value to this measure.
* @param value the measurement to record.
* @param distContext the distContext associated with the measurements.
* @param spanContext the {@link SpanContext} that identifies the {@link Span}
* for which the measurements are associated with.
*/
record(value: number): void;
record(value: number, distContext: DistributedContext): void;
record(
value: number,
distContext: DistributedContext,
spanContext: SpanContext
): void;
record(value: number, spanContext: SpanContext): void;
}
14 changes: 1 addition & 13 deletions packages/opentelemetry-api/src/metrics/Metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

import { DistributedContext } from '../distributed_context/DistributedContext';
import { SpanContext } from '../trace/span_context';

/**
Expand Down Expand Up @@ -123,18 +122,7 @@ export interface MetricUtils {
*/
record(value: number, labelSet: LabelSet): void;

record(
value: number,
labelSet: LabelSet,
distContext: DistributedContext
): void;

record(
value: number,
labelSet: LabelSet,
distContext: DistributedContext,
spanContext: SpanContext
): void;
record(value: number, labelSet: LabelSet, spanContext: SpanContext): void;
}

/**
Expand Down
20 changes: 4 additions & 16 deletions packages/opentelemetry-api/src/metrics/NoopMeter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import { Meter } from './Meter';
import { MetricOptions, Metric, Labels, LabelSet, MetricUtils } from './Metric';
import { BoundMeasure, BoundCounter, BoundGauge } from './BoundInstrument';
import { DistributedContext } from '../distributed_context/DistributedContext';
import { SpanContext } from '../trace/span_context';

/**
Expand Down Expand Up @@ -119,18 +118,11 @@ export class NoopGaugeMetric extends NoopMetric<BoundGauge>

export class NoopMeasureMetric extends NoopMetric<BoundMeasure>
implements Pick<MetricUtils, 'record'> {
record(
value: number,
labelSet: LabelSet,
distContext?: DistributedContext,
spanContext?: SpanContext
) {
if (typeof distContext === 'undefined') {
record(value: number, labelSet: LabelSet, spanContext?: SpanContext) {
if (typeof spanContext === 'undefined') {
this.bind(labelSet).record(value);
} else if (typeof spanContext === 'undefined') {
this.bind(labelSet).record(value, distContext);
} else {
this.bind(labelSet).record(value, distContext, spanContext);
this.bind(labelSet).record(value, spanContext);
}
}
}
Expand All @@ -148,11 +140,7 @@ export class NoopBoundGauge implements BoundGauge {
}

export class NoopBoundMeasure implements BoundMeasure {
record(
value: number,
distContext?: DistributedContext,
spanContext?: SpanContext
): void {
record(value: number, spanContext?: SpanContext): void {
return;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import * as assert from 'assert';
import { NoopTracer, NOOP_SPAN, SpanKind } from '../../src';
import { Context } from '@opentelemetry/scope-base';

describe('NoopTracer', () => {
it('should not crash', () => {
Expand All @@ -38,8 +39,9 @@ describe('NoopTracer', () => {
assert.deepStrictEqual(tracer.getCurrentSpan(), NOOP_SPAN);
const httpTextFormat = tracer.getHttpTextFormat();
assert.ok(httpTextFormat);
httpTextFormat.inject(spanContext, 'HttpTextFormat', {});
assert.deepStrictEqual(httpTextFormat.extract('HttpTextFormat', {}), null);

httpTextFormat.inject(Context.ROOT_CONTEXT, {});
assert.deepStrictEqual(httpTextFormat.extract(Context.ROOT_CONTEXT, {}), Context.ROOT_CONTEXT);

const binaryFormat = tracer.getBinaryFormat();
assert.ok(binaryFormat);
Expand Down
1 change: 1 addition & 0 deletions packages/opentelemetry-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
},
"dependencies": {
"@opentelemetry/api": "^0.4.0",
"@opentelemetry/scope-base": "^0.4.0",
"semver": "^6.3.0"
}
}
Loading