Clarify and rename IsRecordingEvents and WithRecordEvents

[iredelmeier]

The emphasis on "events" in IsRecordingEvents and WithRecordEvents suggests that the methods pertain specifically to events in the sense of logs/what gets added with these methods, when these are really more about whether the span will get exported.

The WithRecordEvents docs also suggest that the default for IsRecordingEvents is false, when in reality it's up to the implementation.

[jmacd]

To me, the most salient piece of information here is "Is this a Real span" vs "Is this a no-op span". I don't like the use of "Recording" to convey this information.

[freeformz]

Heh. Up until now I was under the impression presented by @iredelmeier (although had nagging suspicions I was wrong, but haven't had a chance to dig in).

If these are really meant to indicate if these spans will get exported or not and that is determined by the spans being "real" vs. "no-op", then Go already has a means of doing this via the type system (possibly via a helper function).

If that's not the case, what else am I missing?

[iredelmeier]

My understanding is that the initial goal of IsRecordingEvents/IsRecording from OpenCensus was to avoid running anything expensive for spans that were unnecessary because either (a) they weren't sampled or (b) z-pages aren't been enabled. There's no way of opting in or out of recording for a given span.

It feels like the "recording" functionality has morphed a certain amount since then, and I'm not sure how much of the evolution has been intentional vs accidental.

Thoughts on removing this functionality for now? (Happy to move to a spec issue or RFC depending on the answers here!)

[jmacd]

I think this deserves a spec issue. I still believe "IsNoopSpan()" tells us exactly what we need--the intention is to have a way of eliding expensive computation intended for diagnostics that are going nowhere.

[bogdandrutu]

@jmacd IsRecording() is exactly !IsNoopSpan(). Usually you want to have positive methods/functions.

[jmacd]

Thanks for clarifying. So, it means what I thought it meant.

[freeformz]

@jmacd Does that mean a change to the name then or something else?

[jmacd]

I don't know. I agree with @bogdandrutu that we want a positive method, not a negative, and in code we're likely to see

  if span.IsRecording() {
     span.SetAttribute(key, someExpensiveValue())
  }

I might suggest "IsReal()". I don't feel strongly here.

[jmacd]

And if the example is like the one above, I'd prefer to have lazy value support.
E.g.,

   span.SetAttribute(key.Lazy(func() interface{} { return someExpensiveValue })

[freeformz]

+1. Lazy evaluation via a function seems like a much better way to deal with the problem of real/noop spans than littering code with if span.IsRecording() { ... }. I'd prefer (at least w/o thinking too much about it) the function return a core.Value instead of an interface{}, like so:

span.SetAttribute(key.Lazy(func() (core.Value, error) {
  // some expensive calculation
  return core.NewIntValue(12), nil }),
)

I also feel that returning an error, as a way to ignore the core.Value, is appropriate.

[jmacd]

Related PR w/ proof-of-concept for lazy and structured values:
jmacd#3
This PR is dead, but I intend to resurrect parts of it one day.

[bogdandrutu]

I don't know how it is in Go but in some languages the code above will generate an unnecessary extra allocation. Or if users are concerned about holding too much memory they also may prefer the isRecording.

Also to clarify a bit IsReal sounds to me that you are associated this with having an implementation available for the API. A valid implementation of the API that is based on Dapper paper for example may return spans that record and spans that do not record based on some internal state.

[jmacd]

I'm not very concerned about this, but if there's a user who is very concerned about memory allocation, I still wouldn't use the "is recording" test. I'd use something like the key.Encode(valueEncoder) method seen in jmacd#3:

   span.SetAttribute(key.Encode(value))

where value implements

func (v *MyValue) Encode(w io.Writer) error) { ... }

or similar. Then there's no allocation and lazy evaluation.

[rghetia]

closing stale issues.