-
Notifications
You must be signed in to change notification settings - Fork 24
Add basic logging #396
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
Add basic logging #396
Conversation
nateprewitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far, just a few initial questions
codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java
Outdated
Show resolved
Hide resolved
codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java
Outdated
Show resolved
Hide resolved
| except Exception as e: | ||
| if context.response is not None: | ||
| logger.exception(f"Exception occurred while handling: {context.response}") | ||
| logger.exception("Exception occurred while handling: %s", context.response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What gets logged with this? Do we have an example response payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, here is an example response:
ERROR:bedrock_runtime.client:Exception occurred while handling: Missing Authentication Token
| endpoint_resolver_parameters = StaticEndpointParams(uri=config.endpoint_uri) | ||
| logger.debug("Calling endpoint resolver with parameters: %s", endpoint_resolver_parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to rationalize this against Alex's other Endpoint work. I hope we don't need to add different log statements for each endpoint resolver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I'm planning on doing a small refactor of the endpoint generator so that this code wouldn't be duplicated between them. (IE - the only thing that should be different between different endpoint generators is the parameter classes and how they are constructed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See refactoring: 2a4e6f0
This should play nicely with the logging above.
| logger.debug("HTTP request to sign: %s", context.transport_request) | ||
| logger.debug( | ||
| "Signer properties: %s", | ||
| auth_option.signer_properties | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can build this to check but if you have a quick answer, what does the output of these look like? I haven't checked our repr's for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signer_properties value would print like a normal python dictionary.
auth_option is either HTTPAuthOption | None and at this point we've checked auth_option isn't None. I've copied over the HTTPAuthOption class below for convenience:
@dataclass(kw_only=True)
class HTTPAuthOption:
"""Auth scheme used for signing and identity resolution."""
# The ID of the scheme to use. This string matches the one returned by
# HttpAuthScheme.scheme_id
scheme_id: str
# Parameters to pass to IdentityResolver.get_identity.
identity_properties: dict[str, Any]
# Parameters to pass to HttpSigner.sign.
signer_properties: dict[str, Any]| ${/hasEventStream} | ||
| ) -> Output: | ||
| logger.debug(f"Making request for operation {operation_name} with parameters: {input}") | ||
| logger.debug('Making request for operation "%s" with parameters: %s', operation_name, input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example:
DEBUG:bedrock_runtime.client:Making request for operation "Converse" with parameters: ConverseInput(model_id='amazon.titan-text-express-v1', messages=[Message(role='user', content=[ContentBlockText(value='Create a list of 3 best songs from the 1990s')])], system=None, inference_config=None, tool_config=None, guardrail_config=None, additional_model_request_fields=None, prompt_variables=None, additional_model_response_field_paths=None, request_metadata=None, performance_config=None)
| logger.debug("HTTP request config: %s", request_config) | ||
| logger.debug("Sending HTTP request: %s", context_with_response.transport_request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example:
DEBUG:bedrock_runtime.client:HTTP request config: HTTPRequestConfiguration(read_timeout=None)
DEBUG:bedrock_runtime.client:Sending HTTP request: HTTPRequest(destination=URI(scheme='https', username=None, password=None, host='bedrock-runtime.us-east-1.amazonaws.com', port=None, path='/model/amazon.titan-text-express-v1/converse', query='', fragment=None), method='POST', fields=Fields(OrderedDict({'content-type': Field(name='Content-Type', value=['application/json'], kind=<FieldPosition.HEADER: 0>), 'content-length': Field(name='Content-Length', value=['98'], kind=<FieldPosition.HEADER: 0>)})))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may need to tweak our reprs for fields and URIs so they're more readable. What we have now isn't great for logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that seems reasonable. Out of curiosity, why modify the __repr__ and not __str__? Based off the docs for __repr__, what we have now is preferred:
If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment).
...
This is typically used for debugging, so it is important that the representation is information-rich and unambiguous.
Seems like __str__ is what we want to modify:
Called by str(object), the default format() implementation, and the built-in function print(), to compute the “informal” or nicely printable string representation of an object. The return value must be a str object.
This method differs from object.repr() in that there is no expectation that str() return a valid Python expression: a more convenient or concise representation can be used.
| request=context_with_response.transport_request, | ||
| request_config=request_config, | ||
| ) | ||
| logger.debug("Received HTTP response: %s", context_with_response.transport_response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example:
DEBUG:bedrock_runtime.client:Received HTTP response: HTTPResponse(status=403, fields=Fields(OrderedDict({'date': Field(name='Date', value=['Fri, 28 Feb 2025 18:54:22 GMT'], kind=<FieldPosition.HEADER: 0>), 'content-type': Field(name='Content-Type', value=['application/json'], kind=<FieldPosition.HEADER: 0>), 'content-length': Field(name='Content-Length', value=['42'], kind=<FieldPosition.HEADER: 0>), 'connection': Field(name='Connection', value=['keep-alive'], kind=<FieldPosition.HEADER: 0>), 'x-amzn-requestid': Field(name='x-amzn-RequestId', value=['af2aa5a0-aeae-406b-8082-c131293508e3'], kind=<FieldPosition.HEADER: 0>), 'x-amzn-errortype': Field(name='x-amzn-ErrorType', value=['MissingAuthenticationTokenException:http://internal.amazon.com/coral/com.amazon.coral.service/'], kind=<FieldPosition.HEADER: 0>)})), reason='Forbidden')
Overview
This PR adds the following:
sensitivetrait can only target "Any shape that is not a service, operation, resource, or member.". We instead now check the targeted shape for thesensitivetrait.Why avoid f-strings in logging?
Using f-strings require Python to evaluate their expressions immediately, meaning that the log message is fully formatted regardless of whether it will be output (e.g., if the log level filters it out). Customers who don't configure logs would still experience the performance hit that comes with evaluating log statements.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.