Skip to content

Conversation

fabriceclementz
Copy link

Description

This PR adds support for the laravel-mongodb library by implementing conditional logic to handle MongoDB-specific query building methods.

Problem

Currently, the auto instrumentation triggers the following error when using Laravel MongoDB models:

PHP Warning:  Illuminate\\Database\\Eloquent\\Builder::getModels(): OpenTelemetry: pre hook threw exception, class=Illuminate\\Database\\Eloquent\\Builder function=getModels message=This method is not supported by MongoDB. Try "toMql()" instead. in Unknown on line 0

This occurs because the instrumentation attempts to call toSql() on MongoDB query builders, which is not supported. MongoDB query builders use toMql() instead to generate MongoDB Query Language statements.

I'm open to suggestions and can make updates based on your feedback. Please let me know if any changes are needed.

@fabriceclementz fabriceclementz requested a review from a team as a code owner July 8, 2025 14:20
Copy link

welcome bot commented Jul 8, 2025

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

Copy link

CLA Not Signed

@ChrisLightfootWild
Copy link
Contributor

So this looks like that package is switching out the builder here and that error comes from here.

I'm hesitant to want to support the decision made by that package to seemingly break the Laravel API. I'm also not familiar with the package either and the reasoning that is in place. Do others have any thoughts on this?

You can alter the behaviour of your models by changing the implementation of this method to return a derived builder which will stringify your query in toSql() instead of throwing an exception.

@fabriceclementz
Copy link
Author

I'm hesitant to want to support the decision made by that package to seemingly break the Laravel API. I'm also not familiar with the package either and the reasoning that is in place. Do others have any thoughts on this?

@ChrisLightfootWild Thx for your answer. I was hesitant too before submitting this PR and I think you're right to be hesitant about this. The OpenTelemetry Laravel package shouldn't be responsible for supporting non-standard Laravel API extensions. The package should focus on standard Laravel patterns and not make assumptions about custom query builder implementations.

For now, I've worked around this issue by extending both the Query Builder and Eloquent Query Builder classes to maintain compatibility with our custom database layer while still getting the benefits of OpenTelemetry instrumentation. This approach keeps the OpenTelemetry package focused on its core responsibility without forcing it to accommodate our specific implementation details.

Here is some example code to make the laravel-mongodb package working with this package:

<?php

namespace App\Database;

use MongoDB\Laravel\Eloquent\Builder;

class EloquentBuilder extends Builder
{
    public function toSql()
    {
        return json_encode($this->toMql());
    }
}

----

<?php

namespace App\Database;

use MongoDB\Laravel\Query\Builder as BaseMongoQueryBuilder;

class QueryBuilder extends BaseMongoQueryBuilder
{
    public function toSql()
    {
        return json_encode($this->toMql());
    }
}

and a base model

namespace App;

use App\Database\EloquentBuilder;
use App\Database\QueryBuilder;
use MongoDB\Laravel\Eloquent\Model as EloquentModel;

class Model extends EloquentModel
{
    public function newEloquentBuilder($query)
    {
        return new EloquentBuilder($query);
    }

    public function newBaseQueryBuilder()
    {
        $conn = $this->getConnection();

        return new QueryBuilder(
            $conn,
            $conn->getQueryGrammar(),
            $conn->getPostProcessor()
        );
    }
}

Thank you for the feedback.

@ChrisLightfootWild
Copy link
Contributor

Closing as this has a manual workaround, per the above comment.

Many thanks @fabriceclementz

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.

2 participants