Cosmos Diagnostics#25678
Conversation
A class that gathers useful diagnostics information.
Added system info to diagnostics. This also updates things related to getting exception data for diagnostics
ghost
added
Cosmos
customer-reported
|
Thank you for your contribution bambriz! We will review the pull request and get back to you soon. |
|
|
Centralized diagnostics to client connection and we are now getting diagnostics right after we get a response
Applied diagnostics to async code. Diagnostics should work for async as it does for sync request.
Added consistency leve;, Operation type, and resource type to Cosmos Diagnostics. Added additional fixes and code cleanup.
Removed left over print statement. Previous update also included query diagnostics.
bambriz
requested review from
JericHunter,
annatisch,
kushagraThapar,
simorenoh,
simplynaveen20,
tjprescott and
xinlian12
as code owners
|
Please put the whole request diagnostic string in the PR description. |
|
I added the full text of the response that corresponds to its respective image. |
jay-most
left a comment
jay-most
left a comment
There was a problem hiding this comment.
This isn't the full diagnostic story. Please refer to Java or .Net's implementation.
|
@bambriz - we should avoid having response body as part of successful request diagnostics. Since the response body is already part of the response, and users can access it directly, it doesn't make sense to include it in the diagnostics. For example, for this diagnostics sample, having body doesn't make sense: And actually for requests that end up in exceptions, we should not call the field Also, can you please attach the contract of the Diagnostics object? (basically all the fields that it will have and the information that those fields will contain). |
Could you elaborate for me what you mean by the full diagnostic story? I am unsure what you mean by that. |
I should be able to do this. I will also attach the diagnostic object contract once I make these changes. |
There was a problem hiding this comment.
please also try to add a test file to verify the responses for successful and error scenarios
please also add a small Changelog entry mentioning this revamp of diagnostics
and please add a small section to the README showing how one would use diagnostics for your responses and errors
Changed the way we got diagnsotics http data. Instead of the original approach, I am now using the azure core http policy. I made a child class and replaced it in client connection. By default it performs the same, but with a new flag it will include additional information the cosmos diagnostics class would include.
|
|
||
| def on_request(self, request): # type: (PipelineRequest) -> None | ||
| if self._diagnostics_mode: | ||
| http_request = request.http_request |
There was a problem hiding this comment.
Most of this logging looks like it's using the existing implementation, which means maintaining lots of duplicate code.
I would recommend something like:
def on_request(self, request):
super().on_request(request)
diagnostics_logging = request.context.setdefault(
"enable_diagnostics",
options.pop("enable_diagnostics, self._enable_diagnostics)
)
if diagnostics_logging and logger.isEnabledFor(logging.INFO):
# Add the Cosmos specific stuff here
There was a problem hiding this comment.
Given the logic of the code, would it make sense to do this? The diagnostics logging includes additional information for headers that the original implementation excludes, then there's also the multirecord option logic. So while the code is very similar it also conflicts with one another. Since we are trying to get all the information we can get while the flag is set, it wouldn't make sense to make the redactions from the original policy. We also did something similar with AAD authentication since the original policy didn't work for what we needed in python cosmos. We made an entirely separate one for that.
…ntry, and changelog for new changes Removed Old cosmos diagnostics code Added Cosmos Http Logging Policy Test as well as readme entry and changelog. Other minor fixes.
|
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
bambriz
requested review from
jay-most
and removed request for
JericHunter,
kushagraThapar,
simplynaveen20,
tjprescott and
xinlian12
| database = client.create_database(DATABASE_NAME, logging_enable=True) | ||
| ``` | ||
|
|
||
| Alternatively, you can log using the CosmosHttpLoggingPolicy, which extends from the azure core HttpLoggingPolicy, by passing in your logger to the `logger` argument. |
There was a problem hiding this comment.
The HttpLoggingPolicy is a developer concept - not really an end user concept - so I would not refer to it by name, just discuss the keyword argument.
| | CosmosHttpLoggingPolicy | SansIOHTTPPolicy | | | | | | ||
| | | | logger | X | X | If specified, it will be used to log information. | | ||
| | | | enable_diagnostics_logging | X | X | Used to enable logging additional diagnostic information. Defaults to `False` | | ||
| You can learn more about the different policies and how they work [here](https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/core/azure-core/CLIENT_LIBRARY_DEVELOPER.md#available-policies) |
There was a problem hiding this comment.
I would remove this line and the table above.
| self._enable_diagnostics_logging = kwargs.pop("enable_diagnostics_logging", None) | ||
| super().__init__(logger, **kwargs) | ||
| if self._enable_diagnostics_logging: | ||
| self.logger = logger or logging.getLogger(__name__) |
There was a problem hiding this comment.
Why do we need this alternative logger?
| return | ||
|
|
||
| try: | ||
| parsed_url = list(urllib.parse.urlparse(http_request.url)) |
There was a problem hiding this comment.
We shouldn't duplicate this section of code - could you point out where this deviates from the original implementation so we can figure out the best way to accommodate it?
There was a problem hiding this comment.
Same comment applies to response handling.
| # Get logger in my context first (request has been retried) | ||
| # then read from kwargs (pop if that's the case) | ||
| # then use my instance logger | ||
| logger = request.context.setdefault("logger", options.pop("logger", self.logger)) |
There was a problem hiding this comment.
Why is this only done in the case of enable_diagnostics_logging?
| @@ -0,0 +1,133 @@ | |||
| # ------------------------------------ | |||
There was a problem hiding this comment.
Looks like an image file was accidentally committed?
|
Closed PR since this is not the solution we're going with. |
7 participants
Description
Adds a new azure core policy that is a child class of HttpLoggingPolicy. By default it does the same actions as HttpLoggingPolicy, but with a flag it will enable it to log additional useful diagnostic information.
Here is an example of a succesful request and respone with diagnostic mode enabled:
Here it is an example with the default HttpLogginPolicy:
And here are two examples with an exception:
same one with default policy:
second example with additional information:
same one with default policy: