Skip to content
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

Slow response times on GAE & Cloud Run #28

Closed
martijnvdgrift opened this issue Mar 18, 2020 · 14 comments
Closed

Slow response times on GAE & Cloud Run #28

martijnvdgrift opened this issue Mar 18, 2020 · 14 comments

Comments

@martijnvdgrift
Copy link

Hey there!

I was wondering whether you guys could help me with an issue I'm facing.
App.php:

$app->withFacades();
$app->register(Kreait\Laravel\Firebase\ServiceProvider::class);

DomainService.php

public static function insertMessage(String $customerId, Message $message)
    {
        $collectionDocument = app('firebase.firestore')
            ->database()
            ->collection(ChatService::$collectionName)
            ->document($customerId);

        return $collectionDocument
            ->collection(ChatService::$subCollectionName)
            ->newDocument()
            ->create($message->jsonSerialize());
    }

When I run this app on Google App Engine (Flex) or Google Cloud Run and invoke this method call via a Rest call, the average response time is around 16 seconds. When running locally, this is around 500ms.

Do you know where the increase in latency might come from?
We've also used https://github.com/firevel/firestore/tree/master/src, which doesn't have this problem, but Firevel is missing other functionalities.

Versions used:
"kreait/laravel-firebase": "^1.5",
"laravel/lumen-framework": "^6.0"

@martijnvdgrift martijnvdgrift changed the title Low response times on GAE & Cloud Run Slow response times on GAE & Cloud Run Mar 18, 2020
@jeromegamez
Copy link
Member

That's an interesting problem! 🤔

Are you using a Service Account JSON file locally and on GAE/GCR? If so, please try leaving the FIREBASE_CREDENTIALS environment variable empty on the Google Infrastructure - this should force the SDK to use auto-discovery and discover the GAE/GCR environment, which should save some of the expensive roundtrips via external internet connections.

I hope this helps!

@martijnvdgrift
Copy link
Author

Hey, thanks for the quick reaction. On local dev, I'm exposing GOOGLE_APPLICATION_CREDENTIALS as Environment variable. On GAE/GCR, I'm not setting any env vars.

@jeromegamez
Copy link
Member

Darn... I was hoping it would be as easy as that. For the moment I have no other ideas, but I will try it out tomorrow on GAE/GCR as well, if I can at least reproduce the issue, I can hopefully also figure out why this is happening. 🤞

@constantijn
Copy link

constantijn commented Mar 19, 2020

Did some digging and i found a fix.
If i open up this file:
https://github.com/kreait/gcp-metadata-php/blob/master/src/GcpMetadata.php

and replace the constructor with:

public function __construct()
{
    $this->client = new \GuzzleHttp\Client();
}

Instead of the auto injected guzzle client then performance returns to normal. 28 seconds down to 200 milliseconds.

As for WHY this is a problem i have no idea, but this does make the problem go away.

Is there anything I can to do help to get this fixed in an official release so I don't have to monkeypatch my php code? :)

Edit:
I should probably add that we found that calls to the GCP meta data service were taking just under 5 seconds each before this fix, and after this they're back down to 2-3 milliseconds each.

@jeromegamez
Copy link
Member

You already did! Thank you for digging into it and finding a solution!

As you're currently already monkeypatching 😅, could you please try modifying the Factory in https://github.com/kreait/firebase-php/blob/master/src/Firebase/Factory.php#L418

from

} elseif ((new GcpMetadata())->isAvailable()) {

to

} elseif ((new GcpMetadata(new Client()))->isAvailable()) {

and see if this yields the same improvements? If so, I'll release a patch release today and look for a more sustainable solution after that.


Some background: I initially planned to rely on Google's packages as less as possible and to make the SDK HTTP-Client-independent (as it's currently tied to Guzzle). Unfortunately, I never came around really doing it, and so, the SDK is currently in this weird state that I'm duplicating some of the functionality that google/auth already provides. I'm not happy with that at all, but I think now could be the time to finally embrace Guzzle and the google/auth package, at least for the 4.x releases 😅

@constantijn
Copy link

Oh, i came across that part of the code in my bug hunting ;)

That won't (completely) work because a few lines above that we call:

        $serviceAccount = $this->getServiceAccount();

That one ends up going into new Discovery\OnGoogleCloudPlatform(), which has the same metadata problem.

@jeromegamez
Copy link
Member

Ah, of course! 🤦‍♂

Could you please add/replace

"kreait/firebase-php": "dev-discovery"

to your project's composer.json to see if this helps? It's not pretty, but if it works, I would consider it good enough ^^

@constantijn
Copy link

Ok, that causes issues:

  1. The application is back to being slow

and 2)

If i run composer install it works just fine but if i run composer install --no-interaction --no-dev it crashes with:

(1/1) ErrorClass 'Kreait\Laravel\Firebase\ServiceProvider' not found

in Application.php line 196
at Application->register('Kreait\Laravel\Firebase\ServiceProvider')in app.php line 82

@jeromegamez
Copy link
Member

Thanks for testing it out! I think the —no-dev issue could be resolved by changing the dependency to dev-discovery as 4.41.1 or by changing the minimum stability - but that‘s not the point, especially if it‘s back at being slow.

I‘ll start working on a better, more permanent solution and let you know once I have something ready. Thanks for your help and your patience!

@constantijn
Copy link

Thanks for the replies so far, and looking forward to being able to rip this ugly monkey patch out of my codebase ;)

@jeromegamez
Copy link
Member

jeromegamez commented Mar 19, 2020

I just released a new version of kreait/gcp-metadata which should be available on Packagist by now as well.

Could you please do a composer update or add "kreait/gcp-metadata": "^1.1" to your composer.json - I hope this will resolve the issue, it's basically the same thing you already did 😅

@constantijn
Copy link

Seems to work :)
Going to do some more testing but I ripped out my monkey patch and the response times on my rest calls are in the 100-200ms range where i expect them to be :)

@constantijn
Copy link

Looks like it works, thank you very much :)

@jeromegamez
Copy link
Member

I'm glad, thanks for the feedback! 🌺

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

No branches or pull requests

3 participants