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

chore: more advanced types for sync/async resource #2

Closed
wants to merge 42 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
799fdfb
docs: fix broken links to pg and pg-pool plugins (#1421)
sergioregueira Aug 13, 2020
b368d32
Pass W3C Trace Context test suite at strictness 1 (#1406)
michaelgoin Aug 13, 2020
8f876a3
chore: more advanced types for sync/async resource
dyladan Aug 13, 2020
cc645c3
chore: lint
dyladan Aug 13, 2020
221ced8
feat: faster span and trace id generation (#1349)
dyladan Aug 13, 2020
f612922
chore(deps): update all non-major dependencies (#1424)
renovate-bot Aug 14, 2020
8224e1f
fix: correlation context propagation extract for a single entry (#1336)
rubenvp8510 Aug 14, 2020
644edc3
Add mapping for 499 http status (#1418)
dengliming Aug 14, 2020
4f991d0
chore: relax review guidelines by 1 (#1430)
dyladan Aug 14, 2020
da0287c
fix(deps): pin dependencies (#1423)
renovate-bot Aug 14, 2020
27e6c2d
chore!: refer to resource labels as attributes (#1419)
mwear Aug 14, 2020
cb58e7a
Fix typo in document. (#1431)
dengliming Aug 17, 2020
2916ecf
feat: Add missing prometheus exports for ValueRecorder, SumObserver &…
paulfairless Aug 17, 2020
d511a0e
chore: fix histogram type documentation (#1427)
TigerHe7 Aug 17, 2020
b884eec
Add Graceful Shutdown Support (#1321)
jonahrosenblum Aug 17, 2020
739462d
chore(deps): update dependency superagent to v6 (#1425)
renovate-bot Aug 17, 2020
1f8c365
Feat: Migrate EC2 Plugin Resource Detector from IMDSv1 to IMDSv2 (#1408)
EdZou Aug 18, 2020
f402596
docs(exporter-jaeger): add "for Node.js" (#1434)
hongbo-miao Aug 18, 2020
6412688
Fix canary builds (#1437)
dyladan Aug 18, 2020
b6f0208
chore: apply sync resource types to metrics
dyladan Aug 19, 2020
d68ff0f
refactor: separate resource detector packages (#1420)
markwolff Aug 19, 2020
ddd84ff
fix: add Hapi and Koa to default supported plugins (#1440)
carolinee21 Aug 19, 2020
a3357d5
docs(exporter-collector): CollectorTransportNode should be CollectorP…
hongbo-miao Aug 20, 2020
a576c3f
fix: ignore non-number value on BaseBoundInstrument.update (#1366)
legendecas Aug 20, 2020
b2eedfb
feat: add ability to filter grpc methods (#1353)
reggiemcdonald Aug 20, 2020
2a8555f
feat: added AWS Beanstalk Plugins Resource Detector (#1385)
EdZou Aug 24, 2020
fb06b5b
Collector split (#1446)
obecny Aug 24, 2020
2ee9f1a
feat: adding possibility of recording exception (#1372)
obecny Aug 24, 2020
7ef38a1
docs: change documentation URL (#1460)
confiq Aug 25, 2020
d8bff27
refactor: rename HttpText to TextMap propagator (#1458)
dengliming Aug 25, 2020
47f4ce1
feat: introduces ability to suppress tracing via context (#1344)
michaelgoin Aug 25, 2020
53f0654
feat: prometheus serializer (#1310)
legendecas Aug 26, 2020
03a8ee1
Merge remote-tracking branch 'origin/master' into sdk_sync_types
dyladan Aug 27, 2020
2052a24
feat(backcompat): @types/node backcompat tests (#1352)
markwolff Aug 27, 2020
5c7753f
feat: tracers provided by the API become useable when provider regist…
dyladan Aug 27, 2020
e440d5e
Fix PerformanceResource finding with relative URLs (#1389)
johnbley Aug 27, 2020
c5294ba
fix: proper histogram boundaries sort (#1475)
AndrewGrachov Aug 28, 2020
40242ae
chore: bump gcp-metadata (#1469)
dyladan Aug 31, 2020
a7a6b7f
Align xhr span name with spec (#1476)
johnbley Aug 31, 2020
b88b95b
Move SpanContext isValid to the API (#1447)
srjames90 Aug 31, 2020
cb6555a
fix: updates ValueRecorder to allow negative values (#1373)
michaelgoin Aug 31, 2020
c06f31e
Merge remote-tracking branch 'origin/master' into sdk_sync_types
dyladan Sep 1, 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
1 change: 0 additions & 1 deletion packages/opentelemetry-exporter-jaeger/src/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

import { Link, CanonicalCode, SpanKind } from '@opentelemetry/api';
import { Resource } from '@opentelemetry/resources';
import { ReadableSpan } from '@opentelemetry/tracing';
import {
hrTimeToMilliseconds,
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-zipkin/src/zipkin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
statusDescriptionTagName,
} from './transform';
import { OT_REQUEST_HEADER } from './utils';
import { Resource, SERVICE_RESOURCE } from '@opentelemetry/resources';
import { SERVICE_RESOURCE } from '@opentelemetry/resources';
/**
* Zipkin Exporter
*/
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-sdk-node/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export class NodeSDK {
if (this._tracerProviderConfig) {
const tracerProvider = new NodeTracerProvider({
...this._tracerProviderConfig.tracerConfig,
resource: (this._detectResources() as unknown) as Resource,
resource: this._detectResources(),
});

tracerProvider.addSpanProcessor(this._tracerProviderConfig.spanProcessor);
Expand Down
11 changes: 9 additions & 2 deletions packages/opentelemetry-tracing/src/BasicTracerProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,18 @@ export class BasicTracerProvider implements api.TracerProvider {

activeSpanProcessor = new NoopSpanProcessor();
readonly logger: api.Logger;
readonly resource: Resource;
readonly resource: Promise<Resource>;

constructor(config: TracerConfig = DEFAULT_CONFIG) {
this.logger = config.logger ?? new ConsoleLogger(config.logLevel);
this.resource = config.resource ?? Resource.createTelemetrySDKResource();
if (config.resource) {
this.resource =
config.resource instanceof Promise
? config.resource
: Promise.resolve(config.resource);
} else {
this.resource = Promise.resolve(Resource.createTelemetrySDKResource());
}
this._config = Object.assign({}, config, {
logger: this.logger,
resource: this.resource,
Expand Down
41 changes: 26 additions & 15 deletions packages/opentelemetry-tracing/src/Span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,14 @@ import {
timeInputToHrTime,
} from '@opentelemetry/core';
import { Resource } from '@opentelemetry/resources';
import { ReadableSpan } from './export/ReadableSpan';
import { Tracer } from './Tracer';
import { SpanProcessor } from './SpanProcessor';
import { Tracer } from './Tracer';
import { TraceParams } from './types';

/**
* This class represents a span.
*/
export class Span implements api.Span, ReadableSpan {
Copy link
Owner

Choose a reason for hiding this comment

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

why did you remove ReadableSpan ?

Copy link
Author

Choose a reason for hiding this comment

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

It can be added back.

export class Span implements api.Span {
// Below properties are included to implement ReadableSpan for export
// purposes but are not intended to be written-to directly.
readonly spanContext: api.SpanContext;
Expand All @@ -41,7 +40,7 @@ export class Span implements api.Span, ReadableSpan {
readonly links: api.Link[] = [];
readonly events: api.TimedEvent[] = [];
readonly startTime: api.HrTime;
resource: Resource;
private _resource: Resource | Promise<Resource>;
readonly instrumentationLibrary: InstrumentationLibrary;
name: string;
status: api.Status = {
Expand Down Expand Up @@ -70,12 +69,15 @@ export class Span implements api.Span, ReadableSpan {
this.kind = kind;
this.links = links;
this.startTime = timeInputToHrTime(startTime);
this.resource = parentTracer.resource;
this._resource = parentTracer.resource;
this.instrumentationLibrary = parentTracer.instrumentationLibrary;
this._logger = parentTracer.logger;
this._traceParams = parentTracer.getActiveTraceParams();
this._spanProcessor = parentTracer.getActiveSpanProcessor();
this._spanProcessor.onStart(this);

this._resolveResource().then(() => {
this._spanProcessor.onStart(this);
});
}

context(): api.SpanContext {
Expand Down Expand Up @@ -171,15 +173,9 @@ export class Span implements api.Span, ReadableSpan {
);
}

if (this.resource instanceof Promise) {
this.resource.then(resource => {
this.resource = resource;
this._spanProcessor.onEnd(this);
});
return;
}

this._spanProcessor.onEnd(this);
this._resolveResource().then(() => {
this._spanProcessor.onEnd(this);
});
}

isRecording(): boolean {
Expand All @@ -194,6 +190,13 @@ export class Span implements api.Span, ReadableSpan {
return this._ended;
}

get resource(): Resource {
Copy link
Author

Choose a reason for hiding this comment

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

If the promise is resolved (after _resolveResources completes), then this will act like a regular sync resource to satisfy the ReadableSpan interface.

Copy link
Owner

Choose a reason for hiding this comment

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

Imagine the following situation

  1. Resource is provided as promise
  2. Start span
  3. Something is checking for resource, then just started span receives a resource which is Resource.createTelemetrySDKResource()
  4. Resource that was provided as a promise has been just resolved
  5. Span ended - export span
  6. Span is being exported with wrong resource instead of the one provided from promise

Another situation

  1. Resource is provided as promise
  2. Start span
  3. Span ended - export span
  4. Span is being exported with wrong resource instead of the one provided from promise
  5. Resource that was provided as a promise has been just resolved - but previously created span has been already exported with wrong resource

Copy link
Author

Choose a reason for hiding this comment

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

Something is checking for resource

Checking for resource isn't even a part of the public API until we get a "Readable Span", which is not the same as a span. It might be easier if we just send a plain object instead of the actual Span with a getter?

Span is being exported with wrong resource instead of the one provided from promise

No, the export will still have the correct resource as the span will not be sent to the exporter until the resource is resolved, and getters don't cache. Try it if you don't believe me.

Another situation

Span is being exported with wrong resource instead of the one provided from promise

Why? The span isn't actually exported until the resource resolves.

Copy link
Author

Choose a reason for hiding this comment

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

?

Copy link
Owner

Choose a reason for hiding this comment

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

I wrote you a message on gitter 4 days ago, please read :)

Copy link
Author

Choose a reason for hiding this comment

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

D: ok thanks

Too many communication channels to keep track of

if (this._resource instanceof Promise) {
return Resource.createTelemetrySDKResource();
}
return this._resource;
}

private _isSpanEnded(): boolean {
if (this._ended) {
this._logger.warn(
Expand All @@ -204,4 +207,12 @@ export class Span implements api.Span, ReadableSpan {
}
return this._ended;
}

private async _resolveResource() {
try {
this._resource = await this._resource;
} catch (err) {
this._logger.error(`Resource failed to resolve: ${err?.message}`);
}
}
}
8 changes: 2 additions & 6 deletions packages/opentelemetry-tracing/src/Tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ export class Tracer implements api.Tracer {
private readonly _sampler: api.Sampler;
private readonly _traceParams: TraceParams;
private readonly _idGenerator: IdGenerator;
public resource: Resource;

public readonly resource: Promise<Resource>;
readonly instrumentationLibrary: InstrumentationLibrary;
readonly logger: api.Logger;

Expand All @@ -56,11 +57,6 @@ export class Tracer implements api.Tracer {
this._traceParams = localConfig.traceParams;
this._idGenerator = config.idGenerator || new RandomIdGenerator();
this.resource = _tracerProvider.resource;
if (this.resource instanceof Promise) {
this.resource.then(resource => {
this.resource = resource;
});
}
this.instrumentationLibrary = instrumentationLibrary;
this.logger = config.logger || new ConsoleLogger(config.logLevel);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-tracing/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export interface TracerConfig {
traceParams?: TraceParams;

/** Resource associated with trace telemetry */
resource?: Resource;
resource?: Resource | Promise<Resource>;

/**
* Generator of trace and span IDs
Expand Down