Skip to content

[DoctrineBridge] Global query time always at 0.00 ms on profiler #52881

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

Merged
merged 1 commit into from
Dec 22, 2023
Merged

[DoctrineBridge] Global query time always at 0.00 ms on profiler #52881

merged 1 commit into from
Dec 22, 2023

Conversation

maximethiry
Copy link

@maximethiry maximethiry commented Dec 4, 2023

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52880
License MIT

(original explanation made from 7.0 branch, now targeting 6.4 cf comments)

The related issue, when i explained the possible solution : #52880

On Symfony\Bridge\Doctrine\DataCollector\DoctrineDataCollector, updated the native return type from int to float for the method getTime.

Context :

    public function getTime(): int
    {
        $time = 0;
        foreach ($this->data['queries'] as $queries) {
            foreach ($queries as $query) {
                $time += $query['executionMS'];
            }
        }

        return $time;
    }

Inside this method, the value of $time was always a float smaller than 0. The transition to native return type (which was correct regarding to old phpdoc type) made the return cast int to the floating value, hence returning always 0.

I updated the return type to float and updated the test to check for the calculus on floating values.
I had to switch to usleep instead of sleep, since the value might not be an int.

@carsonbot

This comment was marked as resolved.

@carsonbot carsonbot changed the title [Bug][DoctrineBridge] Global query time always at 0.00 ms on profiler [DoctrineBridge] Global query time always at 0.00 ms on profiler Dec 4, 2023
@nicolas-grekas
Copy link
Member

Can you please rebase the PR on 6.4 to change the annotation there instead?
We will handle the case for 7.0 while merging that PR on 6.4 into 7.0.

@maximethiry
Copy link
Author

Thanks for the feedback, will do the rebase 👍
Should I keep the test for floats values, after moving it in a more appropriate function as requested by @derrabus ?

@derrabus
Copy link
Member

derrabus commented Dec 5, 2023

Yes, please keep the test!

@maximethiry maximethiry changed the base branch from 7.0 to 6.4 December 6, 2023 21:03
@maximethiry maximethiry requested a review from derrabus December 6, 2023 21:08
@derrabus derrabus modified the milestones: 7.0, 6.4 Dec 22, 2023
@derrabus
Copy link
Member

Thank you @maximethiry.

@derrabus derrabus merged commit cf85540 into symfony:6.4 Dec 22, 2023
@derrabus
Copy link
Member

We will handle the case for 7.0 while merging that PR on 6.4 into 7.0.

Done.

xabbuh added a commit that referenced this pull request Dec 22, 2023
This PR was merged into the 6.4 branch.

Discussion
----------

Fix expected return type file

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

After #52881

Commits
-------

ad8096b Fix expected return type
This was referenced Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants