chore: refactoring sdk start so that it can be a real sync #1400

obecny · 2020-08-06T23:37:26Z

Which problem is this PR solving?

After this PR the code can look like this

sdk.start();
const http = require('http');

start is not async anymore and resources are being propagated correctly

codecov · 2020-08-06T23:48:53Z

Codecov Report

Merging #1400 into master will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1400      +/-   ##
==========================================
+ Coverage   94.02%   94.24%   +0.22%     
==========================================
  Files         104      145      +41     
  Lines        3448     4330     +882     
  Branches      720      881     +161     
==========================================
+ Hits         3242     4081     +839     
- Misses        206      249      +43

Impacted Files	Coverage Δ
...ges/opentelemetry-exporter-zipkin/src/transform.ts	`100.00% <ø> (ø)`
...es/opentelemetry-metrics/src/BaseObserverMetric.ts	`100.00% <ø> (ø)`
...s/opentelemetry-metrics/src/BatchObserverMetric.ts	`93.10% <ø> (ø)`
...ackages/opentelemetry-metrics/src/CounterMetric.ts	`100.00% <ø> (ø)`
packages/opentelemetry-metrics/src/Meter.ts	`95.91% <ø> (ø)`
...ackages/opentelemetry-metrics/src/MeterProvider.ts	`100.00% <ø> (ø)`
...ges/opentelemetry-metrics/src/SumObserverMetric.ts	`100.00% <ø> (ø)`
...s/opentelemetry-metrics/src/UpDownCounterMetric.ts	`100.00% <ø> (ø)`
...entelemetry-metrics/src/UpDownSumObserverMetric.ts	`100.00% <ø> (ø)`
...s/opentelemetry-metrics/src/ValueObserverMetric.ts	`100.00% <ø> (ø)`
... and 54 more

dyladan · 2020-08-07T13:18:36Z

I don't understand what the advantage is. Having this sometimes sync and sometimes async seems like a confusing decision to explain to customers.

austinlparker · 2020-08-07T13:35:03Z

This benefits SDK consumers who are writing wrappers around NodeSDK and wish to use it in a similar fashion to NodeTracerProvider. With async behavior, I'd need to do something like -

sdk.start().then(() => {
  const express = require('express');
  const app = express()
  // etc etc
});

Allowing for sync means I can create a SDK instance in another module and export it at the top level without having to mess with callbacks.

You can see the difference in these examples - this (https://glitch.com/edit/#!/generated-tender-snarl) uses NodeSDK, and this (https://glitch.com/edit/#!/petite-verbena-playground) uses NodeTracerProvider.

dyladan · 2020-08-10T19:45:56Z

I would prefer to have it either always sync or always async. Having it sometimes sync is setting users up for confusion and failure. I would prefer to make async detectResources() a separate method and make start() always sync.

obecny · 2020-08-10T19:58:19Z

I would prefer to have it either always sync or always async. Having it sometimes sync is setting users up for confusion and failure. I would prefer to make async detectResources() a separate method and make start() always sync.

But this doesn't solve the issue.
From a user perspective I want to make this

const sdk = new NodeSDK({
  autoDetectResources: false,
});
const express = require('express');
const app = express()
const tracer = opentelemetry.trace.getTracer('otel-client-example');
const span = tracer.startSpan('client.ping');
span.end();

To achieve this I think we could delegate to start detecting resources in some "global object whatever we call it , resource detector, resource provider, etc.", it can be also attached to opentelemetry something like "opentelemetry.getResource which will be an observable in case the resource will change after some time - this in fact might be a true)
Then for example processor would be responsible for checking if resources have been already resolved, otherwise it will subscribe for it until it finishes. Up to this moment all spans will be added to temporary "queue" (similar even how the batch processor is working, or exporter collector when using grpc and until it starts".
When resource detection are resolved we can attach the resource back to finished spans and then move them to exporter.
All of this will happen behind the scene and user will have no problem with waiting for anything like this. I'm happy to show it in PR.

dyladan · 2020-08-10T20:15:09Z

Discussion moved to #1410

…nt for the user

…k_sync

mwear

This approach looks good to me! It allows SDK start to be synchronous and spares the export pipeline from having to await the resource by ensuring the promise resolves before letting data through.

mwear · 2020-08-11T22:59:41Z

packages/opentelemetry-sdk-node/README.md

 ```

 ## Configuration

 Below is a full list of configuration options which may be passed into the `NodeSDK` constructor;

-### autoDetectResources


Are we sure we want to remove this option completely? While I think most will want to autodetect resources, I could see wanting to opt-out in some environments.

dyladan

This looks really good. I definitely prefer to allow the sdk to be sync so this is a big improvement.

Just a minor note that doesn't block this PR that we will still have the load order issue which can cause some resource detectors to prevent wrapping. To be fixed in a follow up PR.

dyladan · 2020-08-12T14:27:58Z

packages/opentelemetry-tracing/src/Span.ts

@@ -171,6 +171,14 @@ export class Span implements api.Span, ReadableSpan {
 );
 }

+ if (this.resource instanceof Promise) {
+ this.resource.then(resource => {


Can we race this against a timeout promise so that we can ensure a slow resource detector does not delay or block tracing?

Where is the fix I don't see it?

dyladan · 2020-08-12T14:28:59Z

packages/opentelemetry-tracing/src/export/ReadableSpan.ts

@@ -39,6 +39,6 @@ export interface ReadableSpan {
 readonly events: TimedEvent[];
 readonly duration: HrTime;
 readonly ended: boolean;
- readonly resource: Resource;
+ readonly resource: Resource | Promise<Resource>;


Does this need to be a Promise? Since we know we will await it in the tracer, I see no reason to complicate the types of the export pipeline

I couldn't find a better way so that it has to be like this or I can remove Resource | Promise<Resource>; from almost everywhere but I will have to cast the resource to unknown and then to Resource. This might be a bit hacky as I will have to fake the typescript but that's the only way that works. So either we have Resource | Promise<Resource>; everywhere or we fake ts and cast it to "Resource" in tests and in sdk

dyladan

I think it should be possible to have TracerProvider and MeterProvider resource property be Promise<Resource> (no need for the | Resource overload I think, Span.resource be Resource | Promise<Resource> (since both are needed here to allow it to resolve), and have ReadableSpan.resource just be type Resource because it is already resolved when it is sent to the export pipeline.

dyladan · 2020-08-13T17:53:31Z

packages/opentelemetry-exporter-jaeger/src/transform.ts

@@ -15,6 +15,7 @@
 */

 import { Link, CanonicalCode, SpanKind } from '@opentelemetry/api';
+import { Resource } from '@opentelemetry/resources';


Unused import?

dyladan · 2020-08-13T17:53:39Z

packages/opentelemetry-exporter-zipkin/src/zipkin.ts

@@ -27,7 +27,7 @@ import {
 statusDescriptionTagName,
 } from './transform';
 import { OT_REQUEST_HEADER } from './utils';
-import { SERVICE_RESOURCE } from '@opentelemetry/resources';
+import { Resource, SERVICE_RESOURCE } from '@opentelemetry/resources';


Unused import?

dyladan · 2020-08-13T17:54:16Z

packages/opentelemetry-metrics/src/Metric.ts

@@ -34,7 +34,7 @@ export abstract class Metric<T extends BaseBoundInstrument>
 private readonly _name: string,
 private readonly _options: api.MetricOptions,
 private readonly _kind: MetricKind,
- readonly resource: Resource,
+ public resource: Resource,


This should be Resource | Promise<Resource>

dyladan · 2020-08-13T17:58:28Z

packages/opentelemetry-metrics/src/Metric.ts

@@ -79,15 +79,22 @@ export abstract class Metric<T extends BaseBoundInstrument>

 getMetricRecord(): Promise<MetricRecord[]> {


If this function is async you can avoid the new Promise stuff and just await

async getMetricRecord(): Promise<MetricRecord[]> { this.resource = this.resource instanceof Promise ? await this.resource : this.resource; return this._instruments.values().map(instrument => ({ descriptor: this._descriptor, labels: instrument.getLabels(), aggregator: instrument.getAggregator(), resource: this.resource, instrumentationLibrary: this.instrumentationLibrary, })); }

dyladan · 2020-08-13T17:59:30Z

packages/opentelemetry-sdk-node/src/sdk.ts

@@ -119,8 +120,34 @@ export class NodeSDK {
 }

 /** Detect resource attributes */
- public async detectResources() {
- this.addResource(await detectResources());
+ private _detectResources(): Promise<Resource> {


Can make this async and use await since it returns a promise anyway.

dyladan · 2020-08-13T18:04:40Z

packages/opentelemetry-sdk-node/src/sdk.ts

@@ -152,7 +180,7 @@ export class NodeSDK {
 if (this._meterProviderConfig) {
 const meterProvider = new MeterProvider({
 ...this._meterProviderConfig,
- resource: this._resource,
+ resource: (this._detectResources() as unknown) as Resource,


dyladan · 2020-08-13T18:05:29Z

packages/opentelemetry-tracing/src/Span.ts

@@ -171,6 +171,14 @@ export class Span implements api.Span, ReadableSpan {
 );
 }

+ if (this.resource instanceof Promise) {
+ this.resource.then(resource => {


Where is the fix I don't see it?

dyladan · 2020-08-13T18:07:20Z

packages/opentelemetry-tracing/src/Tracer.ts

@@ -56,6 +56,11 @@ 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) {


Not needed here. Only the span needs to wait for the real resource

dyladan · 2020-08-13T18:07:28Z

packages/opentelemetry-tracing/src/Tracer.ts

@@ -39,7 +39,7 @@ export class Tracer implements api.Tracer {
 private readonly _sampler: api.Sampler;
 private readonly _traceParams: TraceParams;
 private readonly _idGenerator: IdGenerator;
- readonly resource: Resource;
+ public resource: Resource;


Suggested change

public resource: Resource;

public resource: Promise<Resource>;

dyladan · 2020-08-13T18:07:47Z

packages/opentelemetry-tracing/src/Span.ts

@@ -41,7 +41,7 @@ export class Span implements api.Span, ReadableSpan {
 readonly links: api.Link[] = [];
 readonly events: api.TimedEvent[] = [];
 readonly startTime: api.HrTime;
- readonly resource: Resource;
+ resource: Resource;


Suggested change

resource: Resource;

resource: Resource | Promise<Resource>;

dyladan · 2020-08-13T18:40:36Z

See an example of what I'm asking for here: obecny#2

obecny · 2020-08-13T22:03:34Z

I think it should be possible to have TracerProvider and MeterProvider resource property be Promise<Resource> (no need for the | Resource overload I think, Span.resource be Resource | Promise<Resource> (since both are needed here to allow it to resolve), and have ReadableSpan.resource just be type Resource because it is already resolved when it is sent to the export pipeline.

I would rather revert it to previous version when there was more casting as it seems like we have different approach to this. I have added the comment on your PR why I think this is wrong

obecny · 2020-09-01T12:44:40Z

@dyladan how is the your progress on that ?

dyladan · 2020-09-01T15:28:56Z

@dyladan how is the your progress on that ?

Oops I thought I had opened it already

dyladan · 2020-09-02T21:10:00Z

Close in favor of #1484?

obecny · 2020-09-04T17:54:36Z

closing this in favour of new PR -> #1484

…ations-node/register" for an agent-like experience (open-telemetry#1400) * feat(auto-instrumentation-agent): added intrumentation-agent package * feat(auto-instrumentation-agent): added api peer dependency * feat(auto-instrumentation-agent): updated test * feat(auto-instrumentation-agent): updated release please config * feat(auto-instrumentation-agent): updated .release-please-manifest * feat(auto-instrumentation-agent): updated test timeout * feat(auto-instrumentation-agent): removed timeout, added pretest script * feat(auto-instrumentation-agent): moved agent to auto-instrumentations-node * feat(auto-instrumentation-agent): reverted release please config * feat(auto-instrumentation-agent): updated README.md * feat(auto-instrumentation-agent): added env variable for resource detector configuration * feat(auto-instrumentation-agent): removed no-process-exit * feat(auto-instrumentation-agent): removed pretest script * feat(auto-instrumentation-agent): fixed lint issues * feat(auto-instrumentation-agent): updated register test * feat(auto-instrumentation-agent): fixed typo in test * feat(auto-instrumentation-agent): using env instead of export * feat(auto-instrumentation-agent): updated eslint.config.js * feat(auto-instrumentation-agent): updated test * feat(auto-instrumentation-agent): using language specific env var name * feat(auto-instrumentation-agent): updated README.md * feat(auto-instrumentation-agent): updated dependencies * feat(auto-instrumentation-agent): fixed detector test * feat(auto-instrumentation-agent): added comment to test app * feat(auto-instrumentation-agent): small refactor * feat(auto-instrumentation-agent): updated log messages * feat(auto-instrumentation-agent): added missing aws detectors * feat(auto-instrumentation-agent): updated tests * feat(auto-instrumentation-agent): updated getResourceDetectorsFromEnv() * feat(auto-instrumentation-agent): revert unnecessary changes * feat(auto-instrumentation-agent): added reference to supported instrumentations * feat(auto-instrumentation-agent): added reference to env var spec * feat(auto-instrumentation-agent): added reference node documentation * feat(auto-instrumentation-agent): added note for unsupported config * feat(auto-instrumentation-agent): added note to the debug section * feat(auto-instrumentation-agent): moved opentelemetry/api to regular dependency * feat(auto-instrumentation-agent): fixed lint issue * feat(auto-instrumentation-agent): re-added peer dep * feat(auto-instrumentation-agent): re-added api dev dep * feat(auto-instrumentation-agent): fixed lint issue * feat(auto-instrumentation-agent): fixed lint issue * feat(auto-instrumentation-agent): updated README.md * feat(auto-instrumentation-agent): added exporters reference in README.md * feat(auto-instrumentation-agent): updated debug notes * feat(auto-instrumentation-agent): updated installation section to include the api dependency * feat(auto-instrumentation-agent): reordered aws resource detectors * feat(auto-instrumentation-agent): removed process.exit(0) * feat(auto-instrumentation-agent): reordered aws resource detectors * feat(auto-instrumentation-agent): replaced type any with actual types * feat(auto-instrumentation-agent): updated utils test * feat(auto-instrumentation-agent): updated register test * feat(auto-instrumentation-agent): updated README.md * feat(auto-instrumentation-agent): updated shutdown logs * feat(auto-instrumentation-agent): fixed sinon issue

chore: refactoring sdk start so taht it can be real sync

ab5fa99

obecny requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, OlivierAlbertini and vmarchaud as code owners August 6, 2020 23:37

obecny self-assigned this Aug 6, 2020

obecny added bug Something isn't working enhancement New feature or request labels Aug 6, 2020

obecny changed the title ~~chore: refactoring sdk start so taht it can be real sync~~ chore: refactoring sdk start so that it can be a real sync Aug 6, 2020

chore: moving test asserts

7b59a90

austinlparker mentioned this pull request Aug 10, 2020

Resource auto-detection and SDK initialization. #1410

Closed

2 tasks

obecny added 6 commits August 11, 2020 01:00

chore: chore fixing problem with async resources, making it transpare…

58fd6a1

…nt for the user

chore: lint

9d58744

Merge branch 'master' into sdk_sync

4823ba5

chore: cleanup

6e414e5

Merge branch 'sdk_sync' of github.com:obecny/opentelemetry-js into sd…

aa276a6

…k_sync

chore: lint

12817ae

obecny force-pushed the sdk_sync branch from 58b15ac to 7422e29 Compare August 10, 2020 23:33

chore: lint

27338e6

obecny force-pushed the sdk_sync branch from 7422e29 to 27338e6 Compare August 10, 2020 23:38

mwear approved these changes Aug 11, 2020

View reviewed changes

dyladan reviewed Aug 12, 2020

View reviewed changes

obecny added 3 commits August 13, 2020 05:25

chore: removing promise from resource type, adding tests

7b0dbe4

chore: reverting back autoDetectResources

6a62108

Merge branch 'master' into sdk_sync

bfd2907

dyladan reviewed Aug 13, 2020

View reviewed changes

obecny closed this Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactoring sdk start so that it can be a real sync #1400

chore: refactoring sdk start so that it can be a real sync #1400

obecny commented Aug 6, 2020 •

edited

Loading

codecov bot commented Aug 6, 2020 •

edited

Loading

dyladan commented Aug 7, 2020

austinlparker commented Aug 7, 2020

dyladan commented Aug 10, 2020

obecny commented Aug 10, 2020 •

edited

Loading

dyladan commented Aug 10, 2020

mwear left a comment

mwear Aug 11, 2020

dyladan left a comment

dyladan Aug 12, 2020

obecny Aug 13, 2020 •

edited

Loading

dyladan Aug 13, 2020

dyladan Aug 12, 2020

obecny Aug 13, 2020 •

edited

Loading

obecny Aug 13, 2020

dyladan left a comment

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan Aug 13, 2020

dyladan commented Aug 13, 2020

obecny commented Aug 13, 2020

obecny commented Sep 1, 2020

dyladan commented Sep 1, 2020

dyladan commented Sep 2, 2020

obecny commented Sep 4, 2020

		@@ -79,15 +79,22 @@ export abstract class Metric<T extends BaseBoundInstrument>

		getMetricRecord(): Promise<MetricRecord[]> {

	public resource: Resource;
	public resource: Promise<Resource>;

chore: refactoring sdk start so that it can be a real sync #1400

chore: refactoring sdk start so that it can be a real sync #1400

Conversation

obecny commented Aug 6, 2020 • edited Loading

Which problem is this PR solving?

codecov bot commented Aug 6, 2020 • edited Loading

Codecov Report

dyladan commented Aug 7, 2020

austinlparker commented Aug 7, 2020

dyladan commented Aug 10, 2020

obecny commented Aug 10, 2020 • edited Loading

dyladan commented Aug 10, 2020

mwear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obecny Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Aug 13, 2020

obecny commented Aug 13, 2020

obecny commented Sep 1, 2020

dyladan commented Sep 1, 2020

dyladan commented Sep 2, 2020

obecny commented Sep 4, 2020

obecny commented Aug 6, 2020 •

edited

Loading

codecov bot commented Aug 6, 2020 •

edited

Loading

obecny commented Aug 10, 2020 •

edited

Loading

obecny Aug 13, 2020 •

edited

Loading

obecny Aug 13, 2020 •

edited

Loading