[sdk/*] Add support for resource source positions #13449

pgavlin · 2023-07-10T20:06:35Z

Add support to the core SDKs for reporting resource source positions.

In each SDK, this is implemented by crawling the stack when a resource
is registered in order to determine the position of the user code that
registered the resource.

This is somewhat brittle in that it expects a call stack of the form:

Resource class constructor
abstract Resource subclass constructor
concrete Resource subclass constructor
user code

This stack reflects the expected class hierarchy of "cloud resource / component resource < customresource/componentresource < resource".

For example, consider the AWS S3 Bucket resource. When user code instantiates a Bucket, the stack will look like
this in NodeJS:

new Resource (/path/to/resource.ts:123:45)
new CustomResource (/path/to/resource.ts:678:90)
new Bucket (/path/to/bucket.ts:987:65)
<user code> (/path/to/index.ts:4:3)

In order to determine the source position, we locate the fourth frame (the <user code> frame).

pulumi-bot · 2023-07-10T20:07:28Z

Changelog

[uncommitted] (2023-07-13)

Features

[sdk/{go,nodejs,python}] Add support for reporting resource source positions
#13449

pgavlin · 2023-07-12T20:50:35Z

Updated the CHANGELOG.

I've structured these changes s.t. the changes to each SDK are in separate commits. Hopefully this makes it easier to review.

sdk/go/pulumi/context.go

justinvp

LGTM otherwise

sdk/nodejs/runtime/resource.ts

sdk/go/pulumi/context_test.go

justinvp · 2023-07-13T01:13:42Z

sdk/nodejs/tests/runtime/langhost/run.spec.ts

+                        assert.strictEqual(sourcePosition.line, 2);
+                        break;
+                    case "component":
+                        assert.strictEqual(sourcePosition.line, 2);
+                        break;


Why are these line 2? Isn't it 16 and 17?

I am confused by this as well. I've had correct results from Typescript files but I haven't tried JS outside of the test harness. I'm wondering if this is related to the harness or if we'll always see this for JS.

sdk/python/lib/test/langhost/source_position/__main__.py

sdk/python/lib/test/langhost/source_position/test_source_position.py

justinvp · 2023-07-13T01:29:14Z

One question, that I didn't get a chance to ask on #13437. From reading the changes in #13437, when a user is creating an MLC, it looks like we won't record the position of where the user created the MLC, but rather, the position where the component is created inside the MLC provider. Is that correct? If so, should we consider making it so that it records the position of where the user is instantiating the MLC instead?

pgavlin · 2023-07-13T02:32:20Z

it looks like we won't record the position of where the user created the MLC, but rather, the position where the component is created inside the MLC provider. Is that correct? If so, should we consider making it so that it records the position of where the user is instantiating the MLC instead?

Ooooh, that is an excellent catch--I believe you're correct about that. Offhand it seems like we could either include the original source position in the construct request and leave it up to the MLC or we could inform the resource monitor about the actual source position and let it handle things. The latter is probably moderately more complex to implement in the engine, but seems like it is likely to be simpler and less error prone overall. Thoughts? cc @Frassle as well.

justinvp · 2023-07-13T05:38:00Z

Offhand it seems like we could either include the original source position in the construct request and leave it up to the MLC or we could inform the resource monitor about the actual source position and let it handle things. The latter is probably moderately more complex to implement in the engine, but seems like it is likely to be simpler and less error prone overall.

The latter approach would be my preference, if we can make it all work in the engine. That way we don't have to go through all the hassle of propagating the value through the construct request and having to update each SDK (that has APIs for defining MLC providers) to handle it.

Frassle · 2023-07-13T08:38:52Z

Agree with Justin.
There's been quite a few scenarios where it's looked like it would be beneficial to spawn a new resource monitor for MCL construct requests so that we can override data on the responses with contextual information. This feels like another case of that.

If when doing a Construct we spawn a new resource monitor that just overrides the SourcePosition of any RegisterResourceRequests that come through it before propagating to the original resource monitor then this is purely an engine side change and MLCs in every language will just work.
That lets us override the Component to say it's SourcePosition is what's in the user program, but also gives us a place to override all other Resources to say their SourcePositions are in the MLC package.

pgavlin · 2023-07-13T18:32:59Z

If when doing a Construct we spawn a new resource monitor that just overrides the SourcePosition of any RegisterResourceRequests that come through it before propagating to the original resource monitor then this is purely an engine side change and MLCs in every language will just work.
That lets us override the Component to say it's SourcePosition is what's in the user program, but also gives us a place to override all other Resources to say their SourcePositions are in the MLC package.

I like the idea of spawning new resource monitors. I'm not sure about overriding all child resources of a remote component. I think that there's value in deep linking into component sources, especially if we can go from a component source position to the component's package to its sources. I think we can start with your suggestion, though, and implement the rest later.

pgavlin · 2023-07-13T18:39:54Z

(I'll follow up MLC bits in another PR)

pgavlin · 2023-07-13T18:40:01Z

bors merge

13449: [sdk/*] Add support for resource source positions r=pgavlin a=pgavlin Add support to the core SDKs for reporting resource source positions. In each SDK, this is implemented by crawling the stack when a resource is registered in order to determine the position of the user code that registered the resource. This is somewhat brittle in that it expects a call stack of the form: - Resource class constructor - abstract Resource subclass constructor - concrete Resource subclass constructor - user code This stack reflects the expected class hierarchy of "cloud resource / component resource < customresource/componentresource < resource". For example, consider the AWS S3 Bucket resource. When user code instantiates a Bucket, the stack will look like this in NodeJS: new Resource (/path/to/resource.ts:123:45) new CustomResource (/path/to/resource.ts:678:90) new Bucket (/path/to/bucket.ts:987:65) <user code> (/path/to/index.ts:4:3) In order to determine the source position, we locate the fourth frame (the `<user code>` frame). Co-authored-by: Pat Gavlin <pat@pulumi.com>

bors · 2023-07-13T19:15:03Z

Canceled.

Frassle · 2023-07-13T19:25:31Z

I think that there's value in deep linking into component sources, especially if we can go from a component source position to the component's package to its sources.

That was my intent. Not to totally override the sub-resources SourcePositions but just to fix them up to be "package://" style.

Add support to the core SDKs for reporting resource source positions. In each SDK, this is implemented by crawling the stack when a resource is registered in order to determine the position of the user code that registered the resource. This is somewhat brittle in that it expects a call stack of the form: - Resource class constructor - abstract Resource subclass constructor - concrete Resource subclass constructor - user code This stack reflects the expected class hierarchy of "cloud resource / component resource < customresource/componentresource < resource". For example, consider the AWS S3 Bucket resource. When user code instantiates a Bucket, the stack will look like this in NodeJS: new Resource (/path/to/resource.ts:123:45) new CustomResource (/path/to/resource.ts:678:90) new Bucket (/path/to/bucket.ts:987:65) <user code> (/path/to/index.ts:4:3) In order to determine the source position, we locate the fourth frame (the `<user code>` frame).

pgavlin · 2023-07-14T00:53:05Z

bors merge

bors · 2023-07-14T01:26:59Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

bors-ok

pgavlin requested review from Frassle, justinvp, lukehoban and Zaid-Ajaj July 10, 2023 20:06

pgavlin force-pushed the pgavlin/source-position-sdks branch 2 times, most recently from de1cf9d to fe93795 Compare July 12, 2023 20:48

Frassle reviewed Jul 12, 2023

View reviewed changes

sdk/go/pulumi/context.go Show resolved Hide resolved

pgavlin requested a review from Frassle July 12, 2023 23:14

justinvp approved these changes Jul 13, 2023

View reviewed changes

pgavlin force-pushed the pgavlin/source-position-sdks branch from 31a7c5a to 556a2b5 Compare July 13, 2023 16:58

pgavlin force-pushed the pgavlin/source-position-sdks branch from 556a2b5 to f950465 Compare July 13, 2023 19:15

pgavlin force-pushed the pgavlin/source-position-sdks branch 4 times, most recently from f548229 to b504e55 Compare July 13, 2023 23:28

pgavlin force-pushed the pgavlin/source-position-sdks branch from b504e55 to 248f78b Compare July 13, 2023 23:46

bors bot merged commit ca041c9 into master Jul 14, 2023
48 of 50 checks passed

bors bot deleted the pgavlin/source-position-sdks branch July 14, 2023 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sdk/*] Add support for resource source positions #13449

[sdk/*] Add support for resource source positions #13449

pgavlin commented Jul 10, 2023 •

edited

pulumi-bot commented Jul 10, 2023 •

edited

pgavlin commented Jul 12, 2023

justinvp left a comment

justinvp Jul 13, 2023

pgavlin Jul 13, 2023

justinvp commented Jul 13, 2023

pgavlin commented Jul 13, 2023

justinvp commented Jul 13, 2023

Frassle commented Jul 13, 2023

pgavlin commented Jul 13, 2023

pgavlin commented Jul 13, 2023

pgavlin commented Jul 13, 2023

bors bot commented Jul 13, 2023

Frassle commented Jul 13, 2023

pgavlin commented Jul 14, 2023

bors bot commented Jul 14, 2023

[sdk/*] Add support for resource source positions #13449

[sdk/*] Add support for resource source positions #13449

Conversation

pgavlin commented Jul 10, 2023 • edited

pulumi-bot commented Jul 10, 2023 • edited

Changelog

[uncommitted] (2023-07-13)

Features

pgavlin commented Jul 12, 2023

justinvp left a comment

Choose a reason for hiding this comment

justinvp Jul 13, 2023

Choose a reason for hiding this comment

pgavlin Jul 13, 2023

Choose a reason for hiding this comment

justinvp commented Jul 13, 2023

pgavlin commented Jul 13, 2023

justinvp commented Jul 13, 2023

Frassle commented Jul 13, 2023

pgavlin commented Jul 13, 2023

pgavlin commented Jul 13, 2023

pgavlin commented Jul 13, 2023

bors bot commented Jul 13, 2023

Frassle commented Jul 13, 2023

pgavlin commented Jul 14, 2023

bors bot commented Jul 14, 2023

pgavlin commented Jul 10, 2023 •

edited

pulumi-bot commented Jul 10, 2023 •

edited