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

pprof: add support for profiler labels #17280

Closed
matloob opened this issue Sep 29, 2016 · 52 comments

Comments

Projects
None yet
10 participants
@matloob
Copy link
Contributor

commented Sep 29, 2016

I propose adding a mechanism to the runtime and profiling code to allow for annotating profile samples with key-value labels. These labels are an already-existing feature of pprof that we do not support yet.

The user interface would look something like this:

    package context

    type ProfileLabel struct {
        Key string
        Value string
    }

    // DoWithLabels calls f with a copy of the parent context with the
    // given labels appended. The set of labels on that context will be set
    // for the duration of the call to f and restored once f returns.
    func DoWithLabels(parent Context, labels []ProfileLabel, f func(ctx Context))

More details in the proposal document

@quentinmit quentinmit modified the milestone: Proposal Oct 4, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Oct 5, 2016

CL https://golang.org/cl/30015 mentions this issue.

gopherbot pushed a commit to golang/proposal that referenced this issue Oct 17, 2016

design: add proposal document for profile labeling
Updates golang/go#17280

Change-Id: I6dcad5c98f6b19635bfce61d75e7052c05985131
Reviewed-on: https://go-review.googlesource.com/30015
Reviewed-by: Austin Clements <austin@google.com>
@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2016

The proposal document has been submitted to the proposal repo. Please take a look.

https://github.com/golang/proposal/blob/master/design/17280-profile-labels.md

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2016

At first glance, I find the high-level API confusing. You're proposing taking the Context type, which already encapsulates a key-value store, and adding a second separate key-value store to it. I especially find the name "DoWithLabels" confusing - I naively expect it to behave similarly to WithValue, but it doesn't.

To my mind, this would be resolved by moving this outside the context package. I think this can just be a regular consumer of the Context type. i.e. we could move this to pprof.DoWithContext, and it can store the profiling labels in a regular Context key using context.WithValue.

It feels unfortunate to me that this has to be DoWithLabels and not just WithLabels returning a new Context. I understand how that is necessary, though, given the runtime API.

The fact that the argument is func(context.Context) means it is cumbersome to pass back any values such as an error from the function. Perhaps the func type should be func(context.Context) error? "Generics would help here".

Finally, don't you need a way to read out the current labels to pass them along with an RPC? It is my understanding that this is one of the underlying goals of the proposal.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2016

I don't see how it makes sense to store all the Context keys as profiling labels.

Personally I think it's fine to use func(context.Context) as the closure can be used for any other values. I don't feel strongly about it, though.

For an RPC I think the expectation is that the specific appropriate labels would be applied by the RPC server. There is no necessary or appropriate correspondence between the complete set of labels used by the client and those used by the server.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2016

I don't have a strong opinion about the package DoWithLabels appears in, but I think context is a sensible place to place it. runtime/pprof is okay also.

I have a slight preference for func(context.Context) just because it's a shorter signature. I think it's a wash between the clunkiness of assigning an error in the closure vs. returning an error and immediately assigning it when DoWithContext returns. But the context is necessary, so what we have now is the bare minimum.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2016

Here's a change with a prototype of the interface (but not actually applying the labels to profiles): https://go-review.googlesource.com/c/31145/

@gopherbot

This comment has been minimized.

Copy link

commented Oct 19, 2016

CL https://golang.org/cl/31145 mentions this issue.

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

@ianlancetaylor Not sure if that was directed at me, but I did not propose storing all the Context keys as profiling labels. I proposed, roughly:

package pprof
type ctxKeyType struct{}
var ctxKey ctxKeyType

func DoWithLabels(ctx context.Context, new Labels, f func(context.Context)) {
  labels := ctx.Value(ctxKey)
  newLabels = mergeLabels(labels, new)
  setLabels(newLabels)
  defer setLabels(labels)
  f(context.WithValue(ctx, ctxKey, newLabels))
}

This can be done in any package, and does not require any change to the Context interface or implementation.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

@quentinmit I think we're all in agreement then... That's exactly how we want DoWithLabels to work. See the prototype here: https://go-review.googlesource.com/c/31145/6/src/context/context.go

I think the only question here is where DoWithLabels belongs. It coulb be in package context or package pprof. I don't have a strong preference.

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

Yep, I think we're all in agreement about how DoWithLabels should work. I still think that it needs a better name and/or a different package, because otherwise I find the current state confusing.

As for reading labels, your proposal currently says:

// ProfileLabels is an immutable map of profiler labels. A nil
// *ProfileLabels is an empty map of labels.
// There is intentionally no way to access the profile labels contained
// inside the ProfLabels because doing so could create a goroutine-local
// storage mechanism.
type ProfileLabels struct { /* runtime-internal unexported fields */ }

From talking to @ianlancetaylor, the thing we are trying to prevent is goroutine-local storage. I missed the fact that runtime.SetProfileLabels returns a pointer to the previous ProfileLabels struct. I don't see why we need that instead of just restoring the labels from the context as my code above does. Then ProfileLabels would not need to be opaque.

Either way, we need to add a way to get at the labels in a context. This could be a func pprof.LabelsFromContext(context.Context) [][2]string or similar (in the same package that DoWithLabels ends up in).

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

@quentinmit That comment is obsolete. I've sent a cl to update it. The "compromise" solution is to allow reading labels, but not on ProfileLabels returned by the runtime. So that allows us to read labels we keep in the context (because they're never set from a ProfileLabels returned by the runtime), while disallowing GLS.

It would be nice to have LabelsFromContext, but I don't think it's strictly necessary (at least for now). You can get all the nice pprof benefits of adding labels without needing to read the labels on a context. It would be really easy to add LabelsFromContext later. -- I'm not saying that we shouldn't add it, but we can (if we want) wait until later to do so.

Another option is to have LabelsFromContext return a runtime.ProfileLabels. Any ProfileLabels set on a context using DoWithLabels should be readable.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

@quentinmit Here's an example of why we'd want to restore the previous ProfileLabels set on the goroutine rather than the parent context's ProfileLabels: Let's say a user wants to do some work that they don't want to have accounted to the current context. They might have code that looks like this:

 DoWithLabels(context.Background(), nil, func(ctx context.Context) { ... } )

When the DoWithLabels returns, they probably want to revert to the context already set on the goroutine rather than the background context.

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

Yeah, makes sense. (But really, don't call it "the context already set on the goroutine"! The runtime is not in fact tracking a Context object. :) This is a great example of the kind of confusion I think putting it in package context will cause.)

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

Eek! That's pretty bad...

I wouldn't mind moving DoWithLabels to package runtime/pprof if @ianlancetaylor and @aclements don't have any objections.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

So the only point of discussion now is which package DoWithLabels belongs in, and maybe whether it should have a different name?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

I've been stressing all along that I believe that since we already have a request-specific type, namely context.Context, that profiling labels should be associated with that type. The user should be able to manage profiling labels entirely in conjunction with a context.Context value, because that is the value that is already being passed to every request-specific functions. I agree that this needs to be carefully documented, but the goal should be for it to work for normal cases without requiring a deeper understanding.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

@ianlancetaylor Is that an argument for adding LabelsFromContext? I think that's something we can do later [because it's not necessary to support profile labels], but I have no objections to adding it to the proposal.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2016

@matloob I have no opinion about LabelsFromContext and I'm not sure I even understand what you are asking. I am only saying what I said above: the user should be able to manage profiling labels entirely in conjunction with a context.Context value. In particular I am rejecting the suggestion above that the user should be expected to call runtime/pprof.DoWithLabels to work with profiling labels.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

@ianlancetaylor Okay. I think I misunderstood your previous comment. Please ignore the stuff about LabelsFromContext.

But I don't see why runtime/pprof.DoWithLabels wouldn't allow the user to manage profiling labels entirely in conjunction with a context.Context value. The only difference betweencontext.DoWithLabels(from the proposal) and the proposalruntime/pprof.DoWithLabels` is the package the function is located in. They'd work with the context type the same way.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

I think we should keep context.DoWithLabels as is. Is that okay?

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

Discussed offline with @ianlancetaylor and @quentinmit. It seems like there's general agreement for naming the function context.DoWithProfileLabels

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Can we wait until we have more experience with it inside Google before adding it to the standard library? Or maybe we do. I haven't been paying attention.

Or could we add it to golang.org/x/net/ctxlabel or something for now, labeling its API as experimental?

I'd prefer not to add runtime API now. In particular, [2]string all over the place is gross. I'd make a type for that. Because this needs runtime support for the profiling bits, perhaps we could do a short-term hack (or GOEXPERIMENT?) to add support for the runtime without add new API for Go 1.8.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2016

I think we have a fair amount of experience with this, but I would also prefer an experimental API if it's possible. What would a short term hack look like? Would we have a flag that switched on part of the API?

I don't have a strong preference between [2]string and struct {key string; value string}.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

As one strawman, you could add some undocumented behavior to runtime/pprof and sneak in some undocumented private interface to any of the interface methods.

Like on,
https://golang.org/pkg/runtime/pprof/#StartCPUProfile
or
https://golang.org/pkg/runtime/pprof/#Profile.Add

Or add a global variable in the package (new API in Go 1.8), like:

   // Experiment is an internal hook for pprof experiments.
   // This is not a documented or stable API.
   var Experiment = experiment{}

And the lowercase experiment type have some public methods you could assert from the golang.org/x/net/ctxlabel package.

@aclements

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

Put it behind a build tag?

In particular, [2]string all over the place is gross. I'd make a type for that.

This was not done without reason. If we use a type, context must use the exact same named type to avoid having to copy the entire structure just to change it from some type in context to some type in runtime (a flaw in the Go conversion rules, IMO, but it is what it is). Alternatively, we could declare a type in the runtime and use that in the context interface, but then you have to import runtime to call the context method (maybe that's okay).

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

I think a named type in the stdlib is okay.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

I spoke to @matloob, @aclements, and @ianlancetaylor about this last week. I believe we agreed that:

  1. The new API can go into runtime/internal/profile, so that it can be used by runtime/pprof but not other clients. This lets us have some flexibility in evolving the API.
  2. The per-goroutine profiling tag should be an opaque value passed into the runtime and then passed back by the runtime when obtaining profiling data. The value needs to be a reference type that participates in garbage collection, or else it is too hard for the client code to GC its own associated state. That reference type could be an interface{} or maybe even an unsafe.Pointer (remember, this API can only be used by runtime/pprof).
  3. The new call for doing something with extra labels can be pprof.Do. Ian had been arguing for a new method on context.Context, but since that's not possible (context.Context is an interface) he agreed that pprof.Do makes sense.

I suggest:

package pprof

func Do(ctx context.Context, labels []Label, func(context.Context))
func Labels(...string) []Label

and then code would look like

pprof.Do(ctx, pprof.Labels("foo", "bar"), func(ctx context.Context) {
    ...
})
@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2016

Ok, then I think we should go with ForLabels, unless anyone has a strong objection. Then we end up with this:

package pprof

type Label struct {
    // No exported fields
}

// WithLabels returns a context with the given labels appended to its
// label set.
func WithLabels(ctx context.Context, labels []Label) context.Context

// SetLabels sets the context's label set onto the current goroutine.
// This is a lower-level API than Do, which should be used instead when possible.
func SetLabels(ctx context.Context)

// Do calls f with a context with the given labels added. It calls SetLabels with that
// context before calling f, and SetLabels with the parent context after.
func Do(ctx context.Context, labels []Label, f func(context.Context)) {
    cctx := WithLabels(ctx, labels)
    SetLabels(cctx)
    defer SetLabels(ctx)
    f(cctx)
    
}

// Labels takes an even number of strings representing key-value pairs
// and adds them to 
func Labels(...string) []Label

// HasLabel reports whether the context has a label set with the given key.
func HasLabel(ctx context.Context, key string) bool

// ForEachLabel invokes cb with each label set on the context.
// cb should return true to continue iteration or false to stop iteration early.
func ForEachLabel(ctx context.Context, cb func(key, value string) bool)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

s/SetLabels/SetGoroutineLabels/ (to discourage accidental use)
s/ForEachLabel/ForLabels/
s/cb/f/
s/cb should/The function f should/ (sentences begin with capital letters)

body of Do should be

defer SetGoroutineLabels(ctx)
ctx = WithLabels(ctx, labels)
SetGoroutineLabels(ctx)
f(ctx)

otherwise this proposal looks good to me.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2016

Okay. With @rsc's changes we end up with:

package pprof

type Label struct {
    // No exported fields
}

// WithLabels returns a context with the given labels appended to its
// label set.
func WithLabels(ctx context.Context, labels []Label) context.Context

// SetGoroutineLabels sets the context's label set onto the current goroutine.
// This is a lower-level API than Do, which should be used instead when possible.
func SetGoroutineLabels(ctx context.Context)

// Do calls f with a context with the given labels added. It calls SetLabels with that
// context before calling f, and SetLabels with the parent context after.
func Do(ctx context.Context, labels []Label, f func(context.Context)) {
    defer SetGoroutineLabels(ctx)
    ctx = WithLabels(ctx, labels)
    SetGoroutineLabels(ctx)
    f(ctx)
}

// Labels takes an even number of strings representing key-value pairs
// and adds them to 
func Labels(...string) []Label

// HasLabel reports whether the context has a label set with the given key.
func HasLabel(ctx context.Context, key string) bool

// ForLabels invokes f with each label set on the context.
// The function f should return true to continue iteration or false to stop iteration early.
func ForLabels(ctx context.Context, f func(key, value string) bool)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

LGTM. Possible we should change type Label and []Label to type LabelList and LabelList (not slice). As written there's nothing particularly useful you can do with the slice, so maybe the sliceness should be opaque.

@rsc rsc modified the milestones: Go1.9Early, Proposal Dec 2, 2016

@rsc rsc changed the title proposal: Add support for pprof profiler labels pprof: add support for profiler labels Dec 2, 2016

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

It seems like the right next steps are:

CL #1: Implementation of and tests for type labels, the functional label set that we'll store in contexts. (Labels, WithLabels, HasLabel, ForLabels)

CL #2: Implementation of and tests for SetLabels and Do, including what happens with new goroutines and goroutines that die.

CL #3: Adding labels to runtime-generated profiles. This is the part that I said I'd help with, if I remember correctly.

It's OK to send those CLs during the Go 1.8 freeze. I will make some time to review them so that they can be ready at the start of Go 1.9.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

When you say that Do calls SetLabels, I assume you mean SetGoroutineLabels.

It's not obvious to me why HasLabel is useful, but if it is useful, it's really not obvious why it doesn't return the value of the label.

@acetechnologist

This comment has been minimized.

Copy link

commented Dec 3, 2016

@ianlancetaylor. I think HasLabel could be useful because in some instances we might need the following behavior: "If set we do nothing. If not set then set with some value."
I guess the same can achieved by iterating thru the labels but I it will be very expensive.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2016

@acetechnologist Well, I guess, but do you have an answer for the second part of the question--why not return the value?

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2016

Discussed with @acetechnologist offline. I don't think there's any reason not to return the value. So HasLabels is renamed to Label and returns the value and an 'ok' bool. I'm also clarifying below that setting a (k, v) where the key already exists will overwrite the previous value for the key (map semantics).

package pprof

type Label struct {
    // No exported fields
}

// WithLabels returns a context with the given labels appended to its
// label set. If labels with a given key already already set on the context, or
// appear more than once in labels, the last label appearing in labels with
// that key will replace the others.
func WithLabels(ctx context.Context, labels []Label) context.Context

// SetGoroutineLabels sets the context's label set onto the current goroutine.
// This is a lower-level API than Do, which should be used instead when possible.
func SetGoroutineLabels(ctx context.Context)

// Do calls f with a context with the given labels added. It calls SetLabels with that
// context before calling f, and SetLabels with the parent context after.
func Do(ctx context.Context, labels []Label, f func(context.Context)) {
    defer SetGoroutineLabels(ctx)
    ctx = WithLabels(ctx, labels)
    SetGoroutineLabels(ctx)
    f(ctx)
}

// Labels takes an even number of strings representing key-value pairs
// and adds them to 
func Labels(...string) []Label

// Label returns the value of the label with the given key on ctx, and a boolean indicating
// whether that label exists.
func Label(ctx context.Context, key string) (string, bool)

// ForLabels invokes f with each label set on the context.
// The function f should return true to continue iteration or false to stop iteration early.
func ForLabels(ctx context.Context, f func(key, value string) bool)
@aclements

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

In terms of documentation, I think we all put quite a bit of effort into wording the documentation on the previous version of this at https://go-review.googlesource.com/c/31145/. Obviously most of that can't be copied verbatim, but rather than starting from scratch, we should at least start from there. For example, the current doc for Do is a inward description of what it does rather than outward description of what it's for. In contrast, the documentation from DoWithLabels from that CL is much better and could mostly be reused here:

// DoWithLabels calls f with a copy of the parent context with the
// given labels added to the parent's label map.
// Each key/value pair in labels is inserted into the label map in the
// order provided, overriding any previous value for the same key.
// The augmented label map will be set for the duration of the call to f
// and restored once f returns.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2016

Let's move forward with the CLs I outlined above and then we can fine-tune the doc comments in the actual code review.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 9, 2016

CL https://golang.org/cl/34198 mentions this issue.

gopherbot pushed a commit that referenced this issue Feb 6, 2017

runtime/pprof: add definitions of profile label types
This change defines WithLabels, Labels, Label, and ForLabels.
This is the first step of the profile labels implemention for go 1.9.

Updates #17280

Change-Id: I2dfc9aae90f7a4aa1ff7080d5747f0a1f0728e75
Reviewed-on: https://go-review.googlesource.com/34198
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Feb 7, 2017

CL https://golang.org/cl/36538 mentions this issue.

@rakyll

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@matloob, could you update https://github.com/golang/proposal/blob/master/design/17280-profile-labels.md with the new API? It is impossible to link to the final proposal at this point.

@bradfitz bradfitz modified the milestones: Go1.9, Go1.9Early May 3, 2017

@gopherbot

This comment has been minimized.

Copy link

commented May 15, 2017

CL https://golang.org/cl/43530 mentions this issue.

@matloob

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Hi, I've sent out a CL to update the proposal document with the actual changes: golang.org/cl/43530

gopherbot pushed a commit that referenced this issue Jul 5, 2017

runtime/pprof: Fix type name in function comment
The name LabelList was changed to LabelSet during the development of the
proposal [1], except in one function comment. This commit fixes that.

Fixes #20905.

[1] #17280

Change-Id: Id4f48d59d7d513fa24b2e42795c2baa5ceb78f36
Reviewed-on: https://go-review.googlesource.com/47470
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators May 15, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.