the protected statement above makes this method only callable within Span or Transaction objects. so Span#dup can still call it. but users won't be able to call it from their codebase anymore.

Is the discussion in https://www.rubyguides.com/2018/10/method-visibility/ relevant to us, in particular (see "Private vs Protected Methods") about preferring private over protected and the performance penalty of using protected?

1. it's a micro difference in most of the Ruby applications. I think the SDK has other places to improve if we start caring about the pure-Ruby computation performance. (I do watch memory consumption though from time to time)
2. the method is called once per transaction, which in most cases equals once per request. so I don't think the micro performance difference would add up to anything worth worrying.

Hmm I had to play a little bit with this to understand it.

$ cat t.rb
class Span
  def do
    set_span_recorder
  end

  private

  def set_span_recorder
    puts "hello"
  end
end

class Transaction < Span
  def tdo
    set_span_recorder
  end
end

span = Span.new
span.do

transaction = Transaction.new
transaction.do
transaction.tdo

$ ruby t.rb
hello
hello
hello

So both private and protected would work here, and since we never need to call Span.set_span_recorder I was curious why you went with protected.

On trying it out and reading the code more closely, I realized that set_span_recorder is really instantiating a new SpanRecorder object, assigning the instance to an instance variable @span_recorder and adding a reference to the current span.

We call the equivalent method init_span_recorder in Python and initSpanRecorder in JS. In Python we define it on Span, but in JS we define it on Transaction, which seems to make more sense to me since we only ever start recording spans from a transaction. In both Python and JS it is the start_transaction method on Hub that initializes the recorder (I don't particularly like how the method leaks into the public API).

If we copy from Python/JS we'd end up making this method public again 😞

The alternative would be to keep it as an internal implementation detail, but probably in transaction.rb, since we only ever need to create a new instance there. Spans need to get a reference to the private/internal span recorder from their parent.

Hmmm this is interesting. What are the use cases for deep cloning a Span / Transaction, why do we implement this?

I forgot the original motivation for it. but generally I want to avoid a mutation on copy being shared with its origin, like:

      span = Sentry::Transaction.new(sampled: true)
      subject.set_span(span)
      copy = subject.dup

      span.start_child

      # the copy probably shouldn't sees the same recorder change, no?
      expect(copy.span.span_recorder.spans.count).to eq(1)

I've written several test cases to prevent sharing mutations between transaction/spans and their copies. so if we decide this is unnecessary and want to drop it, I prefer to do it in another PR.

Definitely something for another PR, sorry for the distraction. It caught my attention since it is the first time I'm looking in more detail in this SDK.

the copy probably shouldn't sees the same recorder change, no?

I don't know what I would expect... both possibilities could be okay depending on what it means to duplicate a span.

I understand and noticed how it is used in tests. In production code, nobody should probably be calling Transaction.dup or Span.dup?! I am genuinely curious for the use cases of duplicating spans I found one.

I see this is used when cloning a Scope. Unfortunately I think the behavior here is not clearly specified in the SDK specs. Python and JS do not perform a deep copy, only shallow, the recorder is shared (whether that is useful or not).