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

Fix orphan bulk decrypt traces #10037

Merged
merged 11 commits into from
Jul 18, 2022
Merged

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Jul 1, 2022

Description

Currently opentracing spans generated by the HTTP client when calling BulkDecrypt endpoint are not parented correctly. This seems to be an issue with not propagating context.Context through the code correctly. I took a stab at propagating it and I can see the spans snapping back into place.

Since we're changing interfaces under sdk/ this is a breaking change. It can be fixed by injecting context.Background() at the call sites.

Example project:

package main

import (
	"github.com/pulumi/pulumi-aws/sdk/v5/go/aws/s3"
	"github.com/pulumi/pulumi/sdk/v3/go/pulumi"
)

func main() {
	pulumi.Run(func(ctx *pulumi.Context) error {
		// Create an AWS resource (S3 Bucket)
		bucket, err := s3.NewBucket(ctx, "my-bucket", nil)
		if err != nil {
			return err
		}

		// Export the name of the bucket
		ctx.Export("bucketName", bucket.ID())
		return nil
	})
}
pulumi up --tracing file:./up.trace --yes
PULUMI_DEBUG_COMMANDS=1 pulumi view-trace ./up.trace
# observe orphan traces around bulkdecrypt calls

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

pkg/backend/filestate/state.go Outdated Show resolved Hide resolved
pkg/cmd/pulumi/import.go Outdated Show resolved Hide resolved
@t0yv0 t0yv0 force-pushed the t0yv0/fix-orphan-bulk-decrypt-traces branch 2 times, most recently from 87414cd to 2537fd9 Compare July 8, 2022 13:43
@t0yv0 t0yv0 added this to the 0.75 milestone Jul 8, 2022
@t0yv0 t0yv0 marked this pull request as ready for review July 8, 2022 15:11
@t0yv0 t0yv0 requested review from Frassle and iwahbe July 8, 2022 15:12
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

This looks good to me in general. See my comments for details.

@t0yv0 t0yv0 force-pushed the t0yv0/fix-orphan-bulk-decrypt-traces branch from 2537fd9 to 390f551 Compare July 14, 2022 21:08
@@ -510,7 +514,7 @@ func newImportCmd() *cobra.Command {
}, imports)

if generateCode {
deployment, err := getCurrentDeploymentForStack(s)
deployment, err := getCurrentDeploymentForStack(ctx, s)
Copy link
Member

Choose a reason for hiding this comment

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

s.Import above can take ctx instead of commandContext() as well

@@ -76,7 +77,7 @@ func newStackImportCmd() *cobra.Command {
// We do, however, now want to unmarshal the json.RawMessage into a real, typed deployment. We do this so
// we can check that the deployment doesn't contain resources from a stack other than the selected one. This
// catches errors wherein someone imports the wrong stack's deployment (which can seriously hork things).
snapshot, err := stack.DeserializeUntypedDeployment(&deployment, stack.DefaultSecretsProvider)
snapshot, err := stack.DeserializeUntypedDeployment(ctx, &deployment, stack.DefaultSecretsProvider)
Copy link
Member

Choose a reason for hiding this comment

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

Again, there's a call to s.ImportDeployment below which can take ctx instead of commandContext()

@t0yv0 t0yv0 merged commit 96a3783 into master Jul 18, 2022
@pulumi-bot pulumi-bot deleted the t0yv0/fix-orphan-bulk-decrypt-traces branch July 18, 2022 13:36
@t0yv0 t0yv0 self-assigned this Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants