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

Missing request Id when we upload offline conversions #1005

Closed
rilwanfit opened this issue Apr 2, 2024 · 17 comments
Closed

Missing request Id when we upload offline conversions #1005

rilwanfit opened this issue Apr 2, 2024 · 17 comments
Labels
question Further information is requested triage Need triage

Comments

@rilwanfit
Copy link

rilwanfit commented Apr 2, 2024

Your question:
We are currently migrating our project from Google Ads API V15 to V16 with the release of Google Ads PHP 22.1.0.

In our application, we used to log the RequestID whenever we sent offline conversions to Google Ads API. (both success calls as well as failed calls)

We used the following code when we used V15.

use Google\Ads\GoogleAds\V15\Services\ConversionUploadServiceClient;

 /** @var UploadClickConversionsResponse $response */
[$response,$status] = $conversionUploadService->uploadClickConversions( 
    $customerId,
    $conversions,
    true,
    ['withResponseMetadata' => true],
);

The $status is the GoogleAdsResponseMetadata where we can get the request-id.

Problem

With V16 the above code is NOT working anymore. So we have updated the code to the following

use Google\Ads\GoogleAds\V16\Services\Client\ConversionUploadServiceClient;

$response = $conversionUploadService->uploadClickConversions(
        UploadClickConversionsRequest::build($customerId, $conversions, true),
 );

With this code, we missed the entire GoogleAdsResponseMetadata object.

How can we get the request Id with new version (V16)? (both success calls as well as failed calls)

@rilwanfit rilwanfit added question Further information is requested triage Need triage labels Apr 2, 2024
@fiboknacky
Copy link
Member

fiboknacky commented Apr 2, 2024

It may be related to the change of new generated code and google/gax. Let me ask the owners of the repository and get back here.

@fiboknacky
Copy link
Member

BTW, what google/gax did you install in your project?

@rilwanfit
Copy link
Author

rilwanfit commented Apr 2, 2024

We are using "googleads/google-ads-php": "^v22.1.0" and google/gax (^1.19.1) as an internal dependency

@bshaffer
Copy link
Collaborator

bshaffer commented Apr 9, 2024

Without looking too deeply (I'm not at a laptop), the first example contains ['withResponseMetadata' => true] and the second does not. Have you tried removing this line?

@bshaffer
Copy link
Collaborator

bshaffer commented Apr 9, 2024

Hmm, after a second thought, I think the issue is that the API has changed, and the second way you have is the new (and correct) way to make the API call, which would mean this is working as designed.

@fiboknacky
Copy link
Member

I don't think so. The API specification itself hasn't changed.

We have GoogleAdsGapicTrait that has been used for a long time with the GAPIC v1 classes.

modifyUnaryCallable and modifyStreamingCallable are the methods heavily used to inject what we need such as GoogleAdsException and to return the metadata like shown above.

The GAPIC v1 classes can call these functions successfully, but somehow for GAPIC v2 classes (like GoogleAdsServiceClient), it looks like there is something changed underneath that makes the result of a callable not propagated back.

Has anything in gax-php changed that would affect how the middleware works for this specific case?

@bshaffer
Copy link
Collaborator

@fiboknacky If I could get a repro test going for this, we could roll back versions of GAX until we land on a version where it works - this would go a long way in helping diagnose this issue

@fiboknacky
Copy link
Member

FYI @rilwanfit

Brent (@bshaffer) and I synced offline and we found the root cause.
Brent has implemented a new solution (#1013), which was merged last week.
I'll release a new client library version soon this week at earliest.

@fiboknacky
Copy link
Member

New version of the client library was released.

@dabontv
Copy link

dabontv commented Jul 3, 2024

Is there an example to get metadata in the new update? It seems the api docs are still outdated https://developers.google.com/google-ads/api/docs/client-libs/php/response-metadata

@fiboknacky
Copy link
Member

We've just updated the doc. Sorry for your inconvenience.

@dabontv
Copy link

dabontv commented Jul 3, 2024

Using v17 example:

        $conversionUploadServiceClient = $googleAdsClient->getConversionUploadServiceClient();
        $response = $conversionUploadServiceClient->uploadClickConversions(
            UploadClickConversionsRequest::build($customerId, $clickConversion, true),
            ['withResponseMetadata' => true]
        );

$conversionUploadServiceClient->getResponseMetadata(); will return null

@fiboknacky
Copy link
Member

I cannot reproduce your issue. It works as expected on my machine.
Could you share the whole code? And what's the result? Can you still get your log and request ID in there?

@ujwaldhakal
Copy link
Contributor

ujwaldhakal commented Sep 27, 2024

Hi @fiboknacky this is not working even after the changes.
Did you try it with Conversion upload client or other form of clients like search stream etc ?

Here are how the code looks like

public function getConversionUploadClient(): ConversionUploadServiceClient
    {

        $oAuth2Credential = (new OAuth2TokenBuilder())
            ->withJsonKeyFilePath($this->config->getJsonFilePath())
            ->withScopes(self::SCOPES)
            ->withImpersonatedEmail($this->config->getImpersonatedEmail())
            ->build();

        $this->googleAdsClient = (new GoogleAdsClientBuilder())
            ->withDeveloperToken($this->config->getDeveloperToken())
            ->withOAuth2Credential($oAuth2Credential)
            ->withLoginCustomerId((int) $this->config->getLoginCustomerId())
            ->withLogger($this->appLogger)
            ->withTransport('grpc')
            ->build();
        
        return $this->googleAdsClient->getConversionUploadServiceClient();
    }
    
    
     $conversionUploadService = $this->googleAdsClient->getConversionUploadClient();
            $clickConversions = $this->convertToClickConversion($customerId, $conversionActionId, $conversions);
            $response = $conversionUploadService->uploadClickConversions(
                UploadClickConversionsRequest::build($customerId, $clickConversions, true),
                ['withResponseMetadata' => true],
            );

            dd($conversionUploadService->getResponseMetadata()); This is always null 

For conversionUploadClient the getResponseMetadata is always null but for search stream apis I get ResponseMedataObject with empty attributes. This one is from the https://developers.google.com/google-ads/api/docs/client-libs/php/response-metadata this example.


$stream = $googleAdsServiceClient->searchStream(
    SearchGoogleAdsStreamRequest::build($customerId, $query),
    ['withResponseMetadata' => true]
);

dd($stream->getResponseMetadata());
This is what I get Google\Ads\GoogleAds\Lib\V17\GoogleAdsResponseMetadata^ {#35039
  -metadata: []
}

@ujwaldhakal
Copy link
Contributor

@dabontv I am still stuck around this issue, were you able to find a workaround?

@dabontv
Copy link

dabontv commented Oct 3, 2024

@dabontv I am still stuck around this issue, were you able to find a workaround?

We set up our own logs (https://developers.google.com/google-ads/api/docs/client-libs/php/logging)

@ujwaldhakal
Copy link
Contributor

https://developers.google.com/google-ads/api/docs/client-libs/php/logging

Thanks for reply.
Using the logs would require additional changes to map each conversion with the request ID.
Now for me, the only option is to wait for this thing to work as previously. We already built around something that could map each conversion with a request ID.
Let's see what @fiboknacky has to say on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested triage Need triage
Projects
None yet
Development

No branches or pull requests

5 participants