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

Add tracing around os.Exec in node provider #10173

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Jul 18, 2022

Description

pulumi-language-nodejs starts node sub-processes to manage Pulumi projects. With these changes the subprocesses are now visible as spans in the Pulumi trace. The tagging convention is taken from pulumi-language-go. The change is small enough we might not need to highlight it in changelog.

Fixes # (issue)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@t0yv0 t0yv0 added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Jul 18, 2022
@t0yv0 t0yv0 self-assigned this Jul 18, 2022
@t0yv0 t0yv0 added this to the 0.75 milestone Jul 18, 2022
Copy link
Contributor

@RobbieMcKinstry RobbieMcKinstry left a comment

Choose a reason for hiding this comment

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

LGTM! Do you know if there's a way we can carry this trace forward in the node code? To get a fuller picture we should also instrument:

  1. Running the Build script.
  2. JIT compilation and interpretation
  3. Labeling the trace with a tag indicating whether we're transpiling or not.

@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Jul 18, 2022

Ah, also, I think this happens on the Go side, but we download and install dependencies too. I'm not sure if we have a trace for that or not. We will need pretty soon, I suspect, since that's likely to be another large speed up after transpilatino/slow booting. There are a number of issues around using Yarn 2 PnP instead of NPM to get a massive boost in download speed.

@t0yv0
Copy link
Member Author

t0yv0 commented Jul 19, 2022

Yes! The https://opentelemetry.io/docs/instrumentation/js/getting-started/nodejs/#setup can be added to the node SDK and then Node can add traces too. We might need to migrate/upgrade opentracing to opentelemetry in the Go SDK first. This starts to sound like a large detour but given how difficult it seems to get an idea what Node is spending time on, maybe we should consider doing it?

@t0yv0 t0yv0 merged commit 161ae7d into master Jul 19, 2022
@pulumi-bot pulumi-bot deleted the t0yv0/add-spans-to-nodejs-lang-provider branch July 19, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants