CRV-1021 implemented sharing correlation id between services

[maletskyi-paysera]

No description provided.

[maletskyi-paysera]

@borilyordanov review it, please

[borilyordanov]

Wouldn't it be better to move this logic out of the CorrelationIdProvider class? For example, we could create an event listener for the kernel.request event, which checks for the presence of the correlation ID in the HTTP headers and then explicitly sets the correlation ID in the CorrelationIdProvider.

[maletskyi-paysera]

I tried to implement your suggestion but realized that Symfony adds some logs even before kernel.request event. It means that it happens before the Request object creation. So not your suggested approach not mine was not working correctly.
The only way to solve this problem I see is by getting the header value directly from the $_SERVER variable. That's why the CorrelationIdFromHeaderExtractor was created.
I realize that using $_SERVER variable directly is something we would like to avoid, so if you have a better solution, let me know.

[borilyordanov]

Let's avoid adding this return type hint, as it would technically be a BC break.

[maletskyi-paysera]

void return type was introduced in PHP 7.1 https://www.php.net/releases/7_1_0.php
Since in composer.json we have "php": "^7.2 || ^8.0" it's not possible to break BC in this case

[borilyordanov]

I'm referring more to cases where a client of the library decides to do something like this:

use Paysera\LoggingExtraBundle\Service\CorrelationIdProvider;

class CustomCorrelationIdProvider extends CorrelationIdProvider
{
    public function incrementIdentifier()
    {
        parent::incrementIdentifier();
    }
}

Now, imagine we release a new version in which the incrementIdentifier() method signature has a void return type. This change will cause the client code to break.

[maletskyi-paysera]

fixed

[borilyordanov]

How about CorrelationIdHttpClientDecorator, what do you think?

[maletskyi-paysera]

changed

[borilyordanov]

Would it be better if we introduced the decorator class without configuring it? This way, we would allow the clients of the bundle to decide when they need the decorated HTTP client and when they do not. However, if we do that we should mention this in the README file, explaining how to use and configure the decorated HTTP client.

[maletskyi-paysera]

done

[borilyordanov]

Since this appears to be a new feature, I think we should bump the MINOR version instead.

[maletskyi-paysera]

changed

[borilyordanov]

We might need to add symfony/http-client-contracts as dependency.

[maletskyi-paysera]

added

[mSprunskas]

• Correlation id is already not ideal term, apparently borrowed from https://www.rabbitmq.com/tutorials/tutorial-six-php which prevents passing it using same terminology as a header through rabbitmq
• Seeing logs from different apps for same identifier is not really backwards compatible and might cause confusion, especially in Sentry

Therefore it would be better to implement cross-app tracking with new identifier name, eg. "some id" (ray id in cloudflare). Also, consider identifier creation on server level (nginx, gateway, etc.) so that app responsibility would be only to pass the value.