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

upstream of transparanty kms logger #195

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Conversation

beejones
Copy link
Contributor

@beejones beejones commented Nov 7, 2024

Extend Logger with LogContext including scope and request id. This allow us to correlate requests with the logs in KMS in order to debug transactions.
The basis of this development was done by @yf23.

  1. Request ID enhancement

    • Reads request ID from KMS endpoint requests from the following fields (in order)
      [
        'x-ms-kms-request-id',
        'x-ms-request-id',
        'x-request-id',
        'request-id',
        'requestid',
      ]
      
    • Use timestamp as request ID if request does not contain request ID.
      • Need to improve: I cannot get uuid package working. Preferrable to generate random UUID.
    • Returns accepted request ID in response header x-ms-kms-request-id
  2. Log Enrichment with LogContext
    Add function scope stack and request ID to KMS log with LogContext. The output format is [requestId=***,scope=***]

2024-10-24T00:08:51.131926Z -0.002 0   [info ][app] c/js/extensions/console.cpp:156 | [INFO] [requestId=12345678,scope=publickeyEndpoint->OhttpPublicKey] Generate OTTP public key for key id: 1
  1. Add / Format the logs for KMS endpoints and policy operations.

@beejones
Copy link
Contributor Author

beejones commented Nov 7, 2024

@beejones beejones changed the title upstream of transparantie kms logger upstream of transparanty kms logger Nov 8, 2024
Copy link
Contributor

@takuro-sato takuro-sato left a comment

Choose a reason for hiding this comment

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

@beejones Did you check how the actual CCF log looks like with B&A services used? If not can you do that please? If it prints out too much warnings maybe we want to consider not putting them as warnings to avoid people from ignoring other important warnings.

@@ -68,7 +71,7 @@ export class AuthenticationService implements IAuthenticationService {
ServiceResult.Failed({
errorMessage: `Error: invalid caller identity (AuthenticationService)-> ${caller.policy}`,
errorType: "AuthenticationError",
}),
}, 400, this.logContext),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need 400 (auth error) here while we already have errorType: "AuthenticationError",. Looks like they are duplicated information?

I'm sure I'm missing something here but also I think using self-defined "AuthenticationError" type (i.e. what we have already) is nicer approach than using http specific code in general. That gives us better flexibility if necessary.

Comment on lines -55 to +64
Logger.debug(
`isValidJwtToken: ${key}, expected: ${policy[key]}, found: ${jwtProp}, ${compliant}`,
Logger.info(
`isValidJwtToken: ${key}, expected: ${policy[key]}, found: ${jwtProp}, ${compliant}`, logContext
Copy link
Contributor

Choose a reason for hiding this comment

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

What's our criteria for using debug and using info?

src/utils/ServiceRequest.ts Show resolved Hide resolved
@takuro-sato
Copy link
Contributor

@takuro-sato
Copy link
Contributor

@takuro-sato
Copy link
Contributor

Ronny triggered new CI: CI · microsoft/privacy-sandbox-dev@9dc0c53. Thanks!

@beejones
image

Looks like printing out some objects is not successful e.g. [DEBUG] [requestId=1731665433037,scope=listpubkeys] Request: [object Object] . Is it expected? If not can you fix it or make an issue to fix later please?

@beejones
Copy link
Contributor Author

We need to sync the codebase of https://github.com/microsoft/azure-privacy-sandbox-kms/ with https://github.com/microsoft/azure-transparent-kms because this repo has important changes which are beneficial for azure-transparent-kms. The PR was needed to upstream big changes in azure-transparent-kms to this repo. Once this PR is merged, it will be possible to merge the changes into azure-transparent-kms with minimal merge conflicts.
Next step:
#201 to address the open points in this review.

@beejones beejones merged commit c79fceb into main Nov 15, 2024
13 checks passed
@beejones beejones deleted the beejones/Add-LogContext-to-Logger branch November 15, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants