pkg/{porter,tracing}, cmd/porter: add tracing instrumentation for list #1864

VinozzZ · 2022-01-19T14:56:30Z

command

What does this change

This PR adds instrumentation for list command. It also adds support for
automatically setting span name during creation by the caller function
name.
Some fixes for unclosed spans is included as well

What issue does it fix

related to #1857

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

Did you write tests?
Did you write documentation?
Did you change porter.yaml or a storage document record? Update the corresponding schema file.
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

Comment with /azp run test-porter-release if a magefile or build script was modified
Comment with /azp run porter-integration if it's a non-trivial PR

command This PR adds instrumentation for list command. It also adds support for automatically setting span name during creation by the caller function name. Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

carolynvs

Looks good, I just have some comments to help improve readability

pkg/porter/apply.go

pkg/porter/list_test.go

pkg/tracing/scopedLogger.go

pkg/porter/list.go

carolynvs · 2022-01-20T18:00:53Z

pkg/tracing/scopedLogger.go


 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
 )

 type ScopedLogger interface {
 	StartSpan(op string, attrs ...attribute.KeyValue) (context.Context, ScopedLogger)
+	StartSpanNamed(attrs ...attribute.KeyValue) (context.Context, ScopedLogger)


When should someone use StartSpan and explicitly name the op vs. using StartSpanNamed and have the op named after the current function?

Also it's not clear to me what StartSpanNamed will do. How about we tweak the name a bit so it's clear that the span will be named after the function, like StartSpanFromFunction or StartSpanNamedByFunction or something else like that?

From my perspective, a span is for a logical unit that usually contains side affect on a state. That often happens within a function. It also helps to create a trace that easily relate back to the code. We will know exactly which function created a span. I also changed the name to StartSpanNamedByCaller. Let me know what do you think?

When would someone use StartSpan vs StartSpanByCaller? Or should we remove StartSpan and never give a custom name to a span?

I was thinking maybe someone would want multiple spans within a complex function and then they will need to manually provide names for them.

Maybe what we could do is to have this interface instead

type ScopedLogger interface { StartSpan(attrs ...attribute.KeyValue) StarSpanByCustomName(op string, attrs ...attribute.KeyValue) ... }

That way the default behavior is to use callerFunc(), and then if someone wants to use a custom name they will use the other one

pkg/tracing/scopedLogger.go

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

carolynvs

Let's think about when a developer would use StartSpan vs StartSpanByCaller and consider if we should just have a single function that uses the callerFunc. Right now I think other devs working on Porter will be unsure when to use each function.

pkg/tracing/logger.go

pkg/porter/list.go

tests/integration/claim_migration_test.go

carolynvs · 2022-01-25T16:03:30Z

I just checked out the failed build, and it's not from your changes. I think it's a flakey test and the next time it runs, it should pass.

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

carolynvs

Everything looks great, I just have one more question (below).

pkg/build/buildkit/buildx.go

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

carolynvs

This is great thank you for setting up some patterns for us to follow as we instrument the rest of Porter! 💯

pkg/tracing/logger_test.go

carolynvs · 2022-01-26T15:21:33Z

/azp run porter-integration

azure-pipelines · 2022-01-26T15:21:44Z

Azure Pipelines successfully started running 1 pipeline(s).

Two pull requests, getporter#1864 and getporter#1851, were merged, and while it didn't cause a merge conflict, they didn't play well together and it broke the build. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>

getporter#1864) * pkg/{porter,tracing}, cmd/porter: add tracing instrumentation for list command This PR adds instrumentation for list command. It also adds support for automatically setting span name during creation by the caller function name. Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * add readability Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * use new interface for creating a span Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> * add unit test Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Two pull requests, getporter#1864 and getporter#1851, were merged, and while it didn't cause a merge conflict, they didn't play well together and it broke the build. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

VinozzZ force-pushed the add-instrumentation branch from 66262af to 7f55bbd Compare January 19, 2022 14:57

pkg/{porter,tracing}, cmd/porter: add tracing instrumentation for list

f977a35

command This PR adds instrumentation for list command. It also adds support for automatically setting span name during creation by the caller function name. Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

VinozzZ force-pushed the add-instrumentation branch from 7f55bbd to f977a35 Compare January 19, 2022 21:18

VinozzZ marked this pull request as ready for review January 19, 2022 21:44

VinozzZ requested review from carolynvs, jeremyrickard and vdice as code owners January 19, 2022 21:44

carolynvs requested changes Jan 20, 2022

View reviewed changes

carolynvs self-assigned this Jan 21, 2022

add readability

ac09e85

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

carolynvs requested changes Jan 25, 2022

View reviewed changes

use new interface for creating a span

765a832

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

carolynvs reviewed Jan 25, 2022

View reviewed changes

pkg/build/buildkit/buildx.go Outdated Show resolved Hide resolved

add unit test

40ea7a0

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

carolynvs approved these changes Jan 26, 2022

View reviewed changes

pkg/tracing/logger_test.go Show resolved Hide resolved

carolynvs merged commit 4d688d2 into getporter:release/v1 Jan 26, 2022

carolynvs mentioned this pull request Jan 26, 2022

Fix bad merge #1872

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/{porter,tracing}, cmd/porter: add tracing instrumentation for list #1864

pkg/{porter,tracing}, cmd/porter: add tracing instrumentation for list #1864

VinozzZ commented Jan 19, 2022 •

edited

Loading

carolynvs left a comment

carolynvs Jan 20, 2022

VinozzZ Jan 24, 2022

carolynvs Jan 25, 2022

VinozzZ Jan 25, 2022

VinozzZ Jan 25, 2022

carolynvs left a comment

carolynvs commented Jan 25, 2022

carolynvs left a comment

carolynvs left a comment

carolynvs commented Jan 26, 2022

azure-pipelines bot commented Jan 26, 2022

pkg/{porter,tracing}, cmd/porter: add tracing instrumentation for list #1864

pkg/{porter,tracing}, cmd/porter: add tracing instrumentation for list #1864

Conversation

VinozzZ commented Jan 19, 2022 • edited Loading

What does this change

What issue does it fix

Notes for the reviewer

Checklist

Reviewer Checklist

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs Jan 20, 2022

Choose a reason for hiding this comment

VinozzZ Jan 24, 2022

Choose a reason for hiding this comment

carolynvs Jan 25, 2022

Choose a reason for hiding this comment

VinozzZ Jan 25, 2022

Choose a reason for hiding this comment

VinozzZ Jan 25, 2022

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs commented Jan 25, 2022

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs commented Jan 26, 2022

azure-pipelines bot commented Jan 26, 2022

VinozzZ commented Jan 19, 2022 •

edited

Loading