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

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

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.

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.

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.

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/

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

@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.

@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.

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).

@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.

@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.

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.)

Eek! That's pretty bad...

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

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

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.

@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.

@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.

@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.

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

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

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.

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}.

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.

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).

I think a named type in the stdlib is okay.

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) {
    ...
})

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.

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)

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.

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.

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.

@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.

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

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)

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.

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

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

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

@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.

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

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