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

Resource auto-detection and SDK initialization. #1410

Closed
1 of 2 tasks
austinlparker opened this issue Aug 10, 2020 · 10 comments
Closed
1 of 2 tasks

Resource auto-detection and SDK initialization. #1410

austinlparker opened this issue Aug 10, 2020 · 10 comments
Labels
Discussion Issue or PR that needs/is extended discussion.

Comments

@austinlparker
Copy link
Member

  • This only affects the JavaScript OpenTelemetry library
  • This may affect other libraries, but I would like to get opinions here first

Hi! I'm writing this to summarize an issue I've had in putting together workshop/training content for OpenTelemetry JS and how Node SDK initialization works. Right now, if you initialize a new NodeSDK, you are required to do so asynchronously - and my understanding is that this is due to resource detection, as it happens async as well. Unfortunately, this leads to some vaguely ugly code -

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

@obecny opened a PR (#1400) to allow for disabling resource auto-detection, which would allow for synchronous usage of the SDK, but this is mostly a band-aid around the core issue, which is that resource detection happens async.

I understand that the process can't be made synchronous, but I wanted to ask why it needed to be something that end users would need to be so cognizant of. Would it be possible, for example, to move resource auto-detection to the processing pipeline, so that provider/sdk initialization isn't affected by it? Something like provider.addSpanProcessor(new ResourceDetector({...})), and maybe even block processing until resources have been detected/a timeout occurs?

@dyladan
Copy link
Member

dyladan commented Aug 10, 2020

This is an issue that has been tickling the back of my brain as well. The async nature of resource detection is unfortunate.

Previously floated ideas to fix this:

  1. Resource is stored on the tracer provider as a Promise and awaited by the exporter
  • This makes it difficult to merge multiple resources together from different detectors
  1. Resource is detected by the exporter
  • This is a problem if you have multiple tracer providers with different resources

I had not considered the idea of a resource detector being a span processor. This breaks the "one resource per tracer provider" mandate of the spec because you could use the same span processor with multiple tracer providers, but it does allow for the use-case which multiple tracer providers is meant to solve (different resources per-tracer-provider).

@obecny @mayurkale22 WDYT about that proposal? That would allow us to make the SDK package init completely sync which simplifies setup as well.

@dyladan dyladan added the Discussion Issue or PR that needs/is extended discussion. label Aug 10, 2020
@obecny
Copy link
Member

obecny commented Aug 10, 2020

I will copy paste my response in PR

From I 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
Copy link
Member

dyladan commented Aug 10, 2020

That code example looks very reasonable to me

which will be an observable in case the resource will change after some time

Resource does not change over time. It is read only.

I'm happy to show it in PR.

Would love to see a PR of what it looks like. I tried to throw together something similar locally and ran into the problem that our SpanProcessor pipeline is completely sync, which is a problem. In order to fix this we need to make it so span processors can perform async operations.

@austinlparker
Copy link
Member Author

This breaks the "one resource per tracer provider" mandate of the spec because you could use the same span processor with multiple tracer providers,

Could always do a deep copy of the "detected resources" object between providers, with some sort of lock to make sure that resource detection completed or timed out. That said, I'm having a hard time thinking of a situation where two providers in the same process would have different detected resources.

@dyladan
Copy link
Member

dyladan commented Aug 10, 2020

I'm having a hard time thinking of a situation where two providers in the same process would have different detected resources.

Join the club. It is in the spec, so we don't really have a choice here unless we feel like crusading against it. @mwear has also harbored similar sentiments.

I recently was going to do just this, but because tracing is so near GA I didn't want to stir that particular pot.

@obecny
Copy link
Member

obecny commented Aug 10, 2020

Fixed in #1400

@austinlparker
Copy link
Member Author

Is there errata about a 1:1 relationship between tracer providers and resources? Because it seems like you could be spec compliant by simply having the resource processor act like a caching layer - each provider would get a separate instance of the resource processor (so you aren't reassigning anything) but it'd pull from the same pool of values.

When used with distributed tracing, a resource can be associated with the TracerProvider when it is created. That association cannot be changed later. When associated with a TracerProvider, all Spans produced by any Tracer from the provider MUST be associated with this Resource.

This doesn't seem to preclude the behavior I just described - admittedly, I'm not the one trying to implement it, but does my logic seem off?

@mwear
Copy link
Member

mwear commented Aug 11, 2020

Just in case it got lost in the shuffle, #1400 has been updated with a variation on 1 from #1410 (comment)

1. Resource is stored on the tracer provider as a Promise and awaited by the exporter

@obecny
Copy link
Member

obecny commented Aug 11, 2020

Just in case it got lost in the shuffle, #1400 has been updated with a variation on 1 from #1410 (comment)

1. Resource is stored on the tracer provider as a Promise and awaited by the exporter

but it doesn't affect exporter, once the span or metrics goes to exporter it already has correct resource - not a promise

@pichlermarc
Copy link
Member

Closing this issue as we now have resources with attributes that resolve async now, so no need to await a promise on sdk.start() anymore. 🙂

PR: #3460
Duplicate issues (in spirit): #2912 / #1533

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issue or PR that needs/is extended discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants