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

Update PSR-22 - Application Tracing #1301

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AAllport
Copy link
Contributor

After far too long, I've moved all the current work from the Google Doc to the Markdown file.
I have double-checked the meta-document, and I am still happy with its content


To track the time a piece of work takes, a user will call `SpanInterface::activate()`.
This will set the start time and push the current span into whichever context propagation system the provider chooses.
Users MAY call `SpanInterface::start()` to populate the span with a start time, but not pushed into context propagation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why two methods? A SpanInterface::start($time = null) would do.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither start nor activate accept the $time parameter.
start sets the start time of the span, activate pushes the span into the context, marking it as the "currently active span" and sets the start time if not already set.

To track the time a piece of work takes, a user will call `SpanInterface::activate()`.
This will set the start time and push the current span into whichever context propagation system the provider chooses.
Users MAY call `SpanInterface::start()` to populate the span with a start time, but not pushed into context propagation.
Once complete, the user would call SpanInterface::finish() to set the end-time and “pop” the span from the current
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Once complete, the user would call SpanInterface::finish() to set the end-time and “pop” the span from the current
Once complete, the user would call `SpanInterface::finish()` to set the end-time and “pop” the span from the current

```php
function imgResize($size=100) {
$span = $this->tracer->createSpan('image.resize')
->setAttribute('size',$size)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->setAttribute('size',$size)
->setAttribute('size', $size)

->setAttribute('size',$size)
->activate();

try{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
try{
try {

->activate();

try{
//Resize the image
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//Resize the image
// Resize the image

}
```

This PSR does not dictate how a span’s internals should be represented, other than it MUST implement the SpanInterface.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This PSR does not dictate how a span’s internals should be represented, other than it MUST implement the SpanInterface.
This PSR does not dictate how a span’s internals should be represented, other than it MUST implement the `SpanInterface`.

At a minimum, this should return an empty array.
Most commonly, providers will return traceParent and baggage headers to pass on to child services.

Providers MUST add the following function signatures to allow data retrieval from their span:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to provide an interface similar to what other PSRs do instead of defining a set of methods w/o it. See https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-7-http-message.md#3-interfaces

Copy link
Contributor Author

@AAllport AAllport May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting these methods do exist in the interface.
I will leave this comment open until I rephrase it slightly to clarify that the paragraph is drawing attention as opposed to an additional requirement.

E.g.

The following methods are aimed at retrieving data from a providers span for programmatic use

- `getChildren(): array;`

The purpose of these functions is to allow data to be read from providers’ spans in a clear, uniform manner.
`SpanInterface::getParent` MAY return null if no parent is present or the span has been instantiated outside the relevant adapter’s methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`SpanInterface::getParent` MAY return null if no parent is present or the span has been instantiated outside the relevant adapter’s methods.
`SpanInterface::getParent()` MAY return null if no parent is present or the span has been instantiated outside the relevant adapter’s methods.


TBD
* **Framework** - An application framework (or micro-framework) that runs a developers code.
Eg. Laravel, Symphony, CakePHP, Slim

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symphony
Symfony

See: [AAllport/psr-tracing - TracerInterface.php](https://github.com/AAllport/psr-tracing/blob/main/src/TracerInterface.php)

SDKs are expected to provide a concrete Tracer via whichever methods are most appropriate for the frameworks they support (eg, PSR-11 container).
Libraries using this PSR SHOULD implement [TracerAwareTrait](#31-tracerawaretrait).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess here we should mention TracerAwareInterface instead of the trait.
I believe that it might be better to avoid the AwareInterface pattern and have just a TracerManager class that can be global. My assumption is that providers are provided as a php extension or as a wrapper over that so exposing it as an global class might fit better. Or inject it in the container and obtain it where is needed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would personally avoid a TracerManager: DI containers can handle it well enough.

Maybe this can be rephrased as:
It is RECOMMENDED for the libraries using this PSR implement TracerAwareInterface in order to ease the injection of a tracer

or something similar

However, Providers SHOULD make every effort to persist attribute data if provided after a span is activated.

To track the time a piece of work takes, a user will call `SpanInterface::activate()`.
This will set the start time and push the current span into whichever context propagation system the provider chooses.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to getCurrentSpan() from TracerInterface? After "activation" the tracer should return it?

- Outcome
- Exceptions

Upon creation, a new span will not be activated or have any attributes set.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to keep mutability to a minimum on value objects.
How about?:

  • activate the span and start time as constructor time
  • have an ending method to fill in the outcome and eventual exceptions, end the time and deactivate the span.
    • I used something in a wrapper like end() and endWithError($exception) to accomplish this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Activate and start are two different operations. In the majority of cases they are executed at the same time and immediately after the construction, but not always:
For example, you can just prepare an HTTP request via a client and construct the span but without firing it or processing it (no start nor activate should be called).
Then, when sending the request, you should call the start method, but you may not call activate because the request is in progress in background. Only when you need to wait and process the response you should call activate (which marks the span as the currently active span in the context).

The same applies for the outcome and exceptions: adding an exception does not mean that the outcome of the operation is not OK and does not imply the ending of the span; just like ending a span without adding any exception does not imply that the operation has been completed without errors.

Outcomes, exceptions and start/end times are completely unrelated concepts and, unfortunately, simplifying them leaves out some useful cases.

@polishdeveloper
Copy link

@AAllport do you have any timeline on this PSR? I'm currently working on tracing and I got excited to learn that there is a PSR standard I could stick to. Is there anything I could help with to push it forward?

@BafS
Copy link

BafS commented Oct 17, 2023

@AAllport what is the status of this PR? Do you plan to work on it still?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants