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 #1050

Open
ujwaldhakal opened this issue Oct 10, 2024 · 13 comments
Open

Missing request Id when we upload offline conversions #1050

ujwaldhakal opened this issue Oct 10, 2024 · 13 comments
Labels
bug Something isn't working triage Need triage

Comments

@ujwaldhakal
Copy link
Contributor

We are currently using v17 with php SDK v24.0.0. The request id is null for the conversion upload client for e.g

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],
            );

            var_dump($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: []
}

There are some other people facing the same issue in this link #1005

@ujwaldhakal ujwaldhakal added bug Something isn't working triage Need triage labels Oct 10, 2024
@fiboknacky
Copy link
Member

Sorry for late reply. I tried to reproduce this on my machine, but I got a response metadata and its request ID as usual.

@ujwaldhakal
Copy link
Contributor Author

getResponseMetadata

Thanks for the reponse. But I couldn't figure out what am I missing here from my end I always get null.
Is there anything I am missing from the code that I used?

@ujwaldhakal
Copy link
Contributor Author

Hey @fiboknacky I think I found the issue
https://github.com/googleads/google-ads-php/blob/v25.0.0/src/Google/Ads/GoogleAds/Lib/V16/UnaryGoogleAdsResponseMetadataCallable.php#L51
for me the options does not have the key withResponseMetadata passed here. If I manually add withResponseMetadata to true then I get the request id.

There is this package from Gax that you are using https://github.com/googleapis/gax-php/blob/v1.34.1/src/GapicClientTrait.php#L581 in this specific line. It removes the withResponseMetadata from options. Let me know if this helps

@fiboknacky
Copy link
Member

Thanks.

There is this package from Gax that you are using https://github.com/googleapis/gax-php/blob/v1.34.1/src/GapicClientTrait.php#L581 in this specific line. It removes the withResponseMetadata from options. Let me know if this helps

I don't see any line that removes withResponseMetadata from your reference above. Could you please confirm?

@ujwaldhakal
Copy link
Contributor Author

hi @fiboknacky this GapicClientTrait.php invokes $optionalArgs = $this->configureCallOptions($optionalArgs); from startCall .
This $this->configureCallOptions($optionalArgs); makes a call to class called CallOptions https://github.com/googleapis/gax-php/blob/main/src/Options/CallOptions.php#L47 in this class fromArray which sets all the value except withResponseMetadata.

The way I tested when was adding dump before and after of this call $optionalArgs = $this->configureCallOptions($optionalArgs); inside startCall

@ujwaldhakal
Copy link
Contributor Author

hi @fiboknacky did you validate based on my finding?

@fiboknacky
Copy link
Member

Sorry for late response. I'll take a look this week.

@fiboknacky
Copy link
Member

@ujwaldhakal I've checked the file you mentioned and I don't see anything related to this issue.
Note that the response metadata is generated in other files listed in this PR.
Would you able to debug your code in those files and check if the response metadata is created there?

@ujwaldhakal
Copy link
Contributor Author

ujwaldhakal commented Dec 11, 2024

hi @fiboknacky I managed to make some time for it.
You are right the file you mentioned is responsible for generating the metadata only when the 'withResponseMetadata' gets available in the options.
That is the problem whole time the options does not contain withResponseMetadata
I tried to dump the options inside file Google\Ads\GoogleAds\Lib\V17\UnaryGoogleAdsResponseMetadataCallable invoke method I get following properties

^ array:5 [
  "headers" => array:1 [
    "x-goog-request-params" => array:1 [
      0 => "customer_id=xxxxx"
    ]
  ]
  "timeoutMillis" => null
  "transportOptions" => []
  "retrySettings" => null
  "audience" => "https://googleads.googleapis.com/"
]

As you can see the withResponseMetadata this is never present in the option even if I set it from the client that's why the responseMetaData is always null.

I can make changes on this file https://github.com/googleapis/gax-php/blob/main/src/Options/CallOptions.php#L47 to check if the array has withResponseMetadata I can set the value so that the UnaryGoogleAdsResponseMetadataCallable class gets this option

WIth this changes it will fix the issue https://github.com/googleapis/gax-php/pull/606/files

@bshaffer
Copy link
Collaborator

bshaffer commented Dec 11, 2024

@fiboknacky what is withResponseMetadata? I don't want to add it to CallOptions because as far as I know, it's not supported as a call option in the GAPIC libraries. Is this some custom extension added by Ads?

Edit: I see it is supported in the custom middleware UnaryGoogleAdsResponseMetadataCallable

As mentioned by @ujwaldhakal, the problem is happening here, where the CallOptions class validates the array, and removes all unknown/unsupported values.

https://github.com/googleapis/gax-php/blob/173f0a97323284f91fd453c4ed7ed8317ecf6cfa/src/GapicClientTrait.php#L701C1-L708C6

You may be able to just add something like this in your GoogleAdsGapicClientTrait to bypass this behavior:

    private function configureCallOptions(array $optionalArgs): array
    {
        return $optionalArgs;
    }

@fiboknacky
Copy link
Member

It was added in #1013.
Let me take a look at this again, but I really couldn't reproduce what you reported @ujwaldhakal.
With the current code, I could get the response metadata without issues.

@bshaffer
Copy link
Collaborator

oh! Wow, I just don't remember that at all 😅

Okay SGTM, please let me know if I can help!

@ujwaldhakal
Copy link
Contributor Author

@fiboknacky for me the responseMetadata is always null unless i modify the package code. Could you share me the code that is working for you please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage Need triage
Projects
None yet
Development

No branches or pull requests

3 participants