Allow setting start time for Span objects

[dstepe]

Reported here:

https://github.com/philkra/elastic-apm-php-agent/pull/121

I suggest adding a Span::startAt(Timer $timer) method and leave the Span::start() as it is.

Does this make sense to apply to transactions as well? Should there be a Timable interface to enforce these methods?

[tsantos84]

Hi @dstepe, I agree with you but with a small change in the method name. I don't see any use case when startAt would be used to express some date in the future, so I prefer to use startedAt (in the past) to represent something that occurred in the past. I'm also agree to use an abstraction to represent time instead of any scalar value. In this case, the Timer class could be auto started when constructing it.

[dstepe]

I think it's a philosophical difference. I try to approach methods with the "tell, don't ask" style. So to me, I would be telling the Span to "start at" the given time, so that comes naturally to me. You make a good point about never starting a span in the future, but that raises a more fundamental question. Timer objects are not a fixed point in time (or should not be considered so). That makes it semantically wrong to stay "started at this timer". Maybe we really mean to say "start with this timer" so the method would be:

public function startWithTimer(Timer $timer): void

What do you think?

[tsantos84]

I've just took a look in your PR and now it is more clear to me your point of view. So, the big question to me in this subject is when the span will start and when it'll stop. Currently we have constructor, start, startWithTimer and stop methods handling time in some way. In my opinion when we have several possibilities to do the same thing it can bring confusion for devs in how they should deal with time in this case. I think we could shorten this contract and keep it as simple as possible.

$span = new Span();
$span->start();
$span->start(microtime()); // optionally we could pass the start time.
$span->stop();

Deal with time is Span's concern and should keep closed for outside world. A custom instance of Timer doesn't brings a big benefit IMO.

And if we want to avoid scalar values, we could use ValueObjects to represent timestamps and pass it to start method.

[dstepe]

I think I want to take this opportunity to flip things around. I'm concerned that both the Span and Transaction classes have too many responsibilities. Given that they are domain events within APM, I propose that their single responsibility should be to represent the event for delivery to APM. That would exclude the responsibility for timing the event and require we implement a new means of creating those events.

I am working on the Laravel package in parallel and it has the need to create Spans in two distinct ways. Not only must it allow the user to create a new span by timing an action, but it must also create spans from DB query events which provide the start time and duration after the event is complete.

I need to think about this some more, but if there is a general consensus on this view, I suggest we make this change now because of the implication to users. I also suggest we do not make this change on this branch. Rather merge this PR and then start a new branch to focus on that change.

For now, I will make the change you suggested above. That's simpler and seems like a good place to be for now.

[dstepe]

This was implemented in the 7.1 release.