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

feat: add debug log when creating client #2265

Merged
merged 19 commits into from
Dec 3, 2024
Merged

feat: add debug log when creating client #2265

merged 19 commits into from
Dec 3, 2024

Conversation

ohmayr
Copy link
Contributor

@ohmayr ohmayr commented Nov 27, 2024

Fixes b/381099246

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Nov 27, 2024
@ohmayr ohmayr force-pushed the initialize-logging branch from bccb525 to 323df6d Compare November 27, 2024 15:44
@parthea parthea self-assigned this Nov 28, 2024
@parthea parthea changed the title feat: add support for debug logging feat: add debug log when creating client Nov 28, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Nov 29, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Dec 2, 2024
@ohmayr ohmayr marked this pull request as ready for review December 2, 2024 22:32
@ohmayr ohmayr requested a review from a team as a code owner December 2, 2024 22:32
@parthea parthea assigned ohmayr and unassigned parthea Dec 2, 2024
"serviceName": "{{ service.meta.address.proto }}",
"universeDomain": getattr(self._client._transport._credentials, "universe_domain", ""),
"credentialType": f"{type(self._client._transport._credentials).__module__}.{type(self._client._transport._credentials).__qualname__}",
"credentialInfo": credential_info,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does credential_info contain actual secrets? In that case, I would use as the value here f"[hash:{hash(credential_info}]" or probably even better, f"[hash:{hashlib.sha256(credential_info}]"

Copy link
Contributor Author

@ohmayr ohmayr Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

credentials_info was added here: googleapis/google-auth-library-python@6f75dd5.

It looks like it can possibly contain a source (path), type, and principal (email). So we don't have anything that we'd want to hash.

@@ -4,6 +4,7 @@
{% import "%namespace/%name_%version/%sub/services/%service/_client_macros.j2" as macros %}
{% import "%namespace/%name_%version/%sub/services/%service/_shared_macros.j2" as shared_macros %}

import logging as std_logging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious: why are you renaming the standard import? We could simply use non-conflicting names for our own constructs/modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a naming conflict in the logging goldens:

import logging
from google.cloud.logging_v2.types import logging

Copy link
Contributor

@parthea parthea Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #2269 for the long term fix which is to avoid generating modules, like this, which conflict with Python3 built-in modules

Copy link
Contributor

@parthea parthea Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One alternative would be to add logging to the reserved names list here.

However doing so would result in a breaking change in other libraries such as google-cloud-build where a field with the name logging exists.
https://github.com/googleapis/google-cloud-python/blob/adea041c1d43b94d11ff9d4d5ba13c0d7898cac2/packages/google-cloud-build/google/cloud/devtools/cloudbuild_v1/types/cloudbuild.py#L3368

The field logging would be renamed to logging_, similar to how license was renamed to license_ in
#2015

The reason is that the reserved terms list is tightly coupled with the logic to alias import statements, and avoid collisions in field names. IOW, we don't have separate lists for just adding an alias to import statements.

@cached_property
def names(self) -> FrozenSet[str]:
"""Return a set of names used by this proto.
This is used for detecting naming collisions in the module names
used for imports.
"""

@property
def name(self) -> str:
"""Used to prevent collisions with python keywords"""
name = self.field_pb.name
return name + "_" if name in utils.RESERVED_NAMES and self.meta.address.is_proto_plus_type else name

"serviceName": "{{ service.meta.address.proto }}",
"universeDomain": getattr(self._transport._credentials, "universe_domain", ""),
"credentialType": f"{type(self._transport._credentials).__module__}.{type(self._transport._credentials).__qualname__}",
"credentialInfo": credential_info,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on hashing this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment.

@parthea
Copy link
Contributor

parthea commented Dec 3, 2024

I'm going to merge this as-is but please feel free to add more comments and we can continue with follow up fixes.

@parthea parthea merged commit 8be95a2 into main Dec 3, 2024
84 checks passed
@parthea parthea deleted the initialize-logging branch December 3, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants