Only use resource_name when customer_id isn't already set

[brendo]

This PR fixes (I think) a bug where the Customer ID is being overridden incorrectly by the resource ID in the logs.

The current bug with this behaviour is the Customer ID is being logged incorrectly for some requests such as checking offline user jobs which have the resource name in this format:

/customers/{customer_id}/offlineUserDataJobs/{job_id}

Right now, the customer_id will be set to job_id, which is wrong.

I think the logic is meant to be, "only use the resource name if the customer ID is not set", rather than defaulting to using the resource name to derive the Customer ID all the time.

This PR attempts to make that change.

[danielfrg]

Hi @brendo, thank you for you contribution!

This code is a bit old so I am not 100% sure by looking at first glance. So I will have to make test example first.

Do you have a quick example of a query where the logs are being incorrectly overwritten.

[brendo]

Here's a little script that reproduces the error (with obviously bad ID's):

customer_id = "1234"
refresh_token = "yyyy"

client = Google::Ads::GoogleAds::GoogleAdsClient.new do |config|
  config.client_id = xxx
  config.client_secret = xxx
  config.developer_token = xxx
  config.refresh_token = refresh_token
  config.linked_customer_id = customer_id
end

# Create User List
operation = client.operation.create_resource.user_list do |ul|
  ul.name = "test user list"
  ul.crm_based_user_list = client.resource.crm_based_user_list_info do |crm|
    crm.upload_key_type = :CONTACT_INFO
  end
end

user_list = client.service.user_list.mutate_user_lists(customer_id: customer_id, operations: [operation])
user_list_resource_name = user_list.results.first.resource_name
# "customers/1234/userLists/8373719475"

# Create Offline Job
offline_user_data_job = client.resource.offline_user_data_job do |job|
  job.type = :CUSTOMER_MATCH_USER_LIST
  job.customer_match_user_list_metadata =
    client.resource.customer_match_user_list_metadata do |m|
      m.user_list = user_list_resource_name
    end
end

offline_job = client.service.offline_user_data_job.create_offline_user_data_job(customer_id: customer_id, job: offline_user_data_job)
offline_job_resource_name = offline_job.resource_name
# "customers/1234/offlineUserDataJobs/33427908943"

# This is the query that produces the wrong output
client.service.offline_user_data_job.run_offline_user_data_job(resource_name: offline_job_resource_name)

The log headers for these three requests are:

This one is fine:

CID: 1234, Host: googleads.googleapis.com:443, Method: /google.ads.googleads.v14.services.UserListService/MutateUserLists, IsFault: no, Request ID: FMh9SOHdtQFL8z6ZOhH2Ow
Outgoing request: Headers: {"developer-token":"REDACTED","linked-customer-id":"1234","x-goog-api-client":"gl-ruby/3.2.2 gccl/23.1.0 gax/0.19.1 gapic/23.1.0 grpc/1.57.0 pb/3.24.2","x-goog-request-params":"customer_id=1234"} Payload: {"customerId":"1234","operations":[{"create":{"name":"test user list 1","crmBasedUserList":{"uploadKeyType":"CONTACT_INFO"}}}]}
Incoming response: Payload: {"results":[{"resourceName":"customers/1234/userLists/8373719475"}]}

This one also fine:

CID: 1234, Host: googleads.googleapis.com:443, Method: /google.ads.googleads.v14.services.OfflineUserDataJobService/CreateOfflineUserDataJob, IsFault: no, Request ID: 9v4IliQqGdd8a-usONqriQ
Outgoing request: Headers: {"developer-token":"REDACTED","linked-customer-id":"1234","x-goog-api-client":"gl-ruby/3.2.2 gccl/23.1.0 gax/0.19.1 gapic/23.1.0 grpc/1.57.0 pb/3.24.2","x-goog-request-params":"customer_id=1234"} Payload: {"customerId":"1234","job":{"type":"CUSTOMER_MATCH_USER_LIST","customerMatchUserListMetadata":{"userList":"customers/1234/userLists/8373719475"}}}
Incoming response: Payload: {"resourceName":"customers/1234/offlineUserDataJobs/33427908943"}

This one busted, CID is the ID of the offlineUserDataJob, not the customerId

CID: 33427908943, Host: googleads.googleapis.com:443, Method: /google.ads.googleads.v14.services.OfflineUserDataJobService/RunOfflineUserDataJob, IsFault: no, Request ID: cSKsG_gZll9nIiUbA6ECRg
Outgoing request: Headers: {"developer-token":"REDACTED","linked-customer-id":"1234","x-goog-api-client":"gl-ruby/3.2.2 gccl/23.1.0 gax/0.19.1 gapic/23.1.0 grpc/1.57.0 pb/3.24.2","x-goog-request-params":"resource_name=customers/1234/offlineUserDataJobs/33427908943"} Payload: {"resourceName":"customers/1234/offlineUserDataJobs/33427908943"}
Incoming response: Payload: {"name":"customers/1234/operations/CjRjdXN0b21lcnMvMTY1NDY4Mzg5NS9vZmZsaW5lVXNlckRhdGFKb2JzLzMzNDI3OTA4OTQzEA4=","done":true}

I'm not sure how to translate that into a unit test 😬

[danielfrg]

Apologies for the delay.

I just confirmed that the PR is indeed the expected behavior, it's what we do in other client libraries.

You can remove the test if it doesn't work and we can merge the PR as is. We will test it in an example.

Finally, can you change the PR to be if/else like we do in Python? https://github.com/googleads/google-ads-python/blob/main/google/ads/googleads/interceptors/logging_interceptor.py#L133-L139 - It makes it a bit more clear

[brendo]

Yes, definitely, I'll make those changes now 👍

[danielfrg]

Thank you!