observe methods from attribute

[brettmc]

Add the capability to observe/hook methods and functions by adding a WithSpan attribute to the function or its interface.
Add the capability to pass function/method arguments to the pre callback by adding a SpanAttribute attribute to a function's parameters or its interface.
The attribute is provided by the extension, as OpenTelemetry\Instumentation\WithSpan and OpenTelemetry\Instumentation\SpanAttribute.

Two extra parameters are passed to pre hooks:

1. an array which can be used to set values on the span: name, span kind
2. an array of function/method arguments to be used as params (from optional SpanAttribute attribute applied to function/method params), and from WithSpan's attribute argument

There is a restriction that WithSpan hooks can only be added to functions/methods that are not already hooked by something else (because the hooks are added at runtime: when a method is executed and has no hooks, we then check for attribute and add hooks).

Lastly, the default pre/post hook methods are \OpenTelemetry\API\Instrumentation\WithSpanHandler::pre and ::post (assuming we would provide a default implementation in our API, which would be a basic version of an auto-instrumentation module). The defaults can be changed via .ini, so folks can roll their own callbacks.

Possible improvements:

• configurable pre/post hooks from ini
• disable feature from ini
• add stubs for new attributes
• disabled by default?
• combine attribute from WithSpan and SpanAttribute into one array?
• support attribute args from interfaces

Issues:

• one minor issue with generating opentelemetry_arginfo.h from stubs. The parser it uses doesn't like attributes, and I also couldn't get it to generate constructors for the attributes. So, I've just implemented as much as (mostly) works, and documented the post-generation hacks.

Closes: open-telemetry/opentelemetry-php#1355

[brettmc]

one more improvement here I thought of today, is that we could also add attributes to control the callback method/s for a function... eg WithPreSpanCallback or something like that? I think it provides greater flexibility than just having one set of callbacks for everything...

[agoallikmaa]

If I'm reading this right, this function seems to collect attributes even if WithSpan was not present. Is there any point populating this if WithSpan was not used? If not, skipping that would save some time on collecting information that will never be used.

[brettmc]

Yes, that's true. I think that 4th param should consider if feature is enabled && WithSpan is used here...

[brettmc]

updated to only collect attributes if feature enabled and WithSpan exists on the function (or an interface) - this lookup could be another instance where keeping track of which function has/hasn't got WithSpan...

[cedricziel]

I love how this evolved - cannot wait to start annotating :)

I think we should reconsider the namespace and rather make it part of OpenTelemetry/API so it can always be available in any project that uses the api package so users dont accidentally run into issues when the add attributes, but the extension is not loaded. Note that would mean we need to maintain BC.

[brettmc]

I think we should reconsider the namespace and rather make it part of OpenTelemetry/API so it can always be available in any project that uses the api package so users dont accidentally run into issues when the add attributes, but the extension is not loaded. Note that would mean we need to maintain BC.

Do you mean that the attributes themselves should live in OpenTelemetry/API namespace, and be provided by the API rather than by the extension? The behaviour when attributes are added but the extension is not installed is that nothing happens. I do think that the attributes would be easier to find if they were in the API, at least. Otherwise, we're relying on IDE stubs...

[agoallikmaa]

Looks good! There is potential future performance optimization work that could be done either proactively or if performance ever comes up as an issue.

[cedricziel]

Do you mean that the attributes themselves should live in OpenTelemetry/API namespace, and be provided by the API rather than by the extension? The behaviour when attributes are added but the extension is not installed is that nothing happens. I do think that the attributes would be easier to find if they were in the API, at least. Otherwise, we're relying on IDE stubs...

Yes, i think the attributes themselves should live in the API package and should be provided by the API package.

My concern is about annotated methods classes that are attempted to be loaded by the autoloader - if the extension is not installed, that will fail because the Attribute classes can not be loaded. So even if users use --ignore-platform-reqs with composer install, any framework that has a compile step like Symfony and Laravel will fail, even in dev and test, simply because the attribute classes will not exist if the extension is missing. Hence my proposal to add the Attribute classes to the API package so they are auto-loadable in any case, even if the extension is not loaded.

IMHO this behavior ensures smoother sailing with modern frameworks. - A comparably small side-effect is that there is no effect if the extension is not loaded, but that is the pure nature of attributes in my opinion and i would prefer any time of the day over imposing the existence of the extension in every single build and deploy step where i need to execute some app code :)

[brettmc]

Yes, i think the attributes themselves should live in the API package and should be provided by the API package.

Discussed at yesterday's SIG and agreed: open-telemetry/opentelemetry-php#1371

[brettmc]

@agoallikmaa @cedricziel - all green again here. Today I have made the following changes which may be worth re-reviewing:

1. removed attributes (and added to the API, as discussed in SIG and above)
2. fixed an issue with generating the hook function from stubs + arginfo
3. tidied tests
4. documentation updates

[jonnyynnoj]

This is really nice, I ended up implementing something similar in my project so it's good to see this become part of the core extension.

One suggestion I have (and I don't know if this is feasible), is whether the extension could look for an interface instead of specifically for the WithSpan/SpanAttribute classes? The interface would define pre/post methods where the actual logic for creating a span, adding an attribute or anything else would live. That'd provide greater flexibility allowing users to define their own attributes and move logic back into PHP land where it's a bit more approachable.

So the WithSpan attribute could look something like this:

#[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_FUNCTION)]
readonly class WithSpan implements \OpenTelemetry\API\Instrumentation\HookAttribute
{
    public function __construct(public ?string $name = null)
    {
    }

    public function pre(mixed $object, array $params, string $class, string $function): void
    {
       // start span
    }

    public function post(array $params, mixed $return, ?\Throwable $exception): void
    {
        // end span
    }
}

[ChrisLightfootWild]

This is really nice, I ended up implementing something similar in my project so it's good to see this become part of the core extension.

One suggestion I have (and I don't know if this is feasible), is whether the extension could look for an interface instead of specifically for the WithSpan/SpanAttribute classes? The interface would define pre/post methods where the actual logic for creating a span, adding an attribute or anything else would live. That'd provide greater flexibility allowing users to define their own attributes and move logic back into PHP land where it's a bit more approachable.

So the WithSpan attribute could look something like this:

#[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_FUNCTION)]
readonly class WithSpan implements \OpenTelemetry\API\Instrumentation\HookAttribute
{
    public function __construct(public ?string $name = null)
    {
    }

    public function pre(mixed $object, array $params, string $class, string $function): void
    {
       // start span
    }

    public function post(array $params, mixed $return, ?\Throwable $exception): void
    {
        // end span
    }
}

There's a mechanism for customising the handlers already, but not to say what you proposed above isn't feasible - I'd have to defer that to the greater minds!