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

fixing the nesting of tracing #2781

Merged
merged 3 commits into from Aug 31, 2023

Conversation

nitishchauhan0022
Copy link
Contributor

What does this change

Fixes the improper nesting of traces which looks like caused by passing of the same context which is returned after logging the config traces, making the following traces as child traces of config trace.

What issue does it fix

Closes #2563

Notes for the reviewer

I have tested the tracing which is mentioned in the issue is properly nested now.

Copy link
Member

@devigned devigned left a comment

Choose a reason for hiding this comment

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

I believe this may cause a regression in the TraceLogger late configuration that was fixed in #2541.

Perhaps, cmd.SetContext(ctx) may need a more selective update for the TraceLogger or other stateful context items, rather than setting the entire context.

@devigned
Copy link
Member

I believe this may cause a regression in the TraceLogger late configuration that was fixed in #2541.

Perhaps, cmd.SetContext(ctx) may need a more selective update for the TraceLogger or other stateful context items, rather than setting the entire context.

I believe the test failure output confirms this theory.

2023/06/10 12:33:34 exec: /tmp/porter1147494806/porter porter plugins install -f plugins.yaml --verbosity=info
    hello_test.go:32: 
        	Error Trace:	/home/vsts/work/1/s/tests/smoke/hello_test.go:32
        	Error:      	"read input file	{"contents": "schemaType: Plugins\nschemaVersion: 1.0.0\nplugins:\n  azure: {}"}
        	            	Downloading https://cdn.porter.sh/plugins/atom.xml to /tmp/porter204193928/atom.xml
        	            	
        	            	Downloading https://cdn.porter.sh/plugins/azure/v1.2.1/azure-linux-amd64 to /tmp/porter1147494806/plugins/azure/azure
        	            	
        	            	Downloading https://cdn.porter.sh/plugins/azure/v1.2.1/azure-linux-amd64 to /tmp/porter1147494806/plugins/azure/runtimes/azure-runtime
        	            	
        	            	installed azure plugin v1.2.1 (64fd786)
        	            	" should not contain "Downloading [https://cdn.porter.sh/plugins/atom.xml"](https://cdn.porter.sh/plugins/atom.xml%22)
        	Test:       	TestHelloBundle
        	Messages:   	Debug information should not have been printed because verbosity is set to info

@nitishchauhan0022
Copy link
Contributor Author

Perhaps, cmd.SetContext(ctx) may need a more selective update for the TraceLogger or other stateful context items, rather than setting the entire context.

Thanks for the guidance, i will look if there are ways to do this.

@schristoff
Copy link
Contributor

Hey @nitishchauhan0022 - thank you so much for your contribution. I wanted to see if you wanted any assistance getting this PR committed, or would you rather someone else try and take it over?
Let us know!

Signed-off-by: ntishchauhan0022 <nitishchauhan0022@gmail.com>
Signed-off-by: ntishchauhan0022 <nitishchauhan0022@gmail.com>
@nitishchauhan0022
Copy link
Contributor Author

@devigned @schristoff Please take a look.

Copy link
Member

@devigned devigned left a comment

Choose a reason for hiding this comment

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

lgtm! Thank you for adding comments describing what is happening with the span changes.

@schristoff, what do you say?

@schristoff
Copy link
Contributor

/azp run porter-integration

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

Thank you so much for this contribution!
Don't forget to make another PR and add your name to our contributors file!
🎉

@schristoff schristoff merged commit 8fbff53 into getporter:main Aug 31, 2023
17 checks passed
schristoff added a commit that referenced this pull request Sep 19, 2023
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.

Improper nesting of porter's trace spans
3 participants