TracingDriverConnectionFactory calls the deprecated method AbstractMySQLPlatform::getName()

[W0rma]

Environment

• PHP 8.2
• sentry/sentry-symfony:4.8.0

Steps to Reproduce

AbstractMySQLPlatform::getName() was deprecated in doctrine/dbal#4749 but is still used in Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverConnectionFactory.

This triggers the following deprecation warning:

AbstractMySQLPlatform::getName() is deprecated. Identify platforms by their class. (AbstractMySQLPlatform.php:1209 called by TracingDriverConnectionF
actory.php:40, https://github.com/doctrine/dbal/issues/4749, package doctrine/dbal)

Doctrine team suggests to use the class name to identify the platform instead.

As far as I can see the only usage of the platform name is in Sentry\SentryBundle\Tracing\Doctrine\DBAL\TracingDriverConnection::getSpanTags().

[Jean85]

Is this a duplicated of #579? In case it would be already fixed in #723

[ste93cry]

No, it isn't. It's a completely different deprecation, which involves the connection class and not the driver class. Actually, I think that the only solution to the problem is to implement a switch that checks the instanceof the platform and returns the expected name for each supported platform, even though it would now be hardcoded and thus less maintainable in the long term.

[ndench]

Would we really need a switch though? We could just use $databasePlatform::class. It just means that the db.system tag would change from mysql to \Doctrine\DBAL\Platforms\MySQL57Platform. Is that a problem?

[ste93cry]

Although not required, I tried to follow as much as possible the guidelines of OpenTelemetry regarding the Connection-level attributes, Since the specification is the best standard out of here in regards to distributed tracing, I think we should continue on this way.

[cleptric]

The list of Sentry's span operations can be found here. It's mandatory to use these, as certain expectations in the product rely on them.

[ste93cry]

We're not talking about span operations, but about span tags 😃

[cleptric]

I see. There is a movement away from creating tags automatically in the SDKs as well, I'll clarify internally. Most likely, we'll move this data to span.data.

[ndench]

@cleptric any updates?

[cleptric]

We should move the span tag values over to span data. As Spans are not indexed in Sentry, this has no impact on the product.

I would be OK to remove $databasePlatform as I don't see it adding this much value.

[ste93cry]

We should move the span tag values over to span data. As Spans are not indexed in Sentry, this has no impact on the product.

I still don't understand what this has to do with this issue. Note I'm not arguing about whether it makes sense or not, but just about the fact that it's a completely different topic.

I would be OK to remove $databasePlatform as I don't see it adding this much value.

It was added because I was trying to follow the OpenTelemetry specification which requires this info to be available. Yes, maybe it doesn't add much value, but sometimes respecting some kind of standard is valuable 😉

[cleptric]

PR is up #743

[AbhiPrasad]

@ste93cry we try to follow the otel conventions for span attributes as much as possible in span data. This is documented here.

Our span data conventions are essentially a superset of otel's, because they are lacking some conventions for mobile/browser and also don't have everything documented for the web server use case also.

[ste93cry]

I assume that your answer is about moving the information from tags to data. As I said before, I don't argue about doing it, to be honest I care little about it, I was just pointing out that the problem being reported in this issue has nothing to do with such change, therefore we should just care about solving the deprecation in the SDK. Any other thing should be done in a different PR.