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 wgc request ID generation & propagation #1507

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

turnabout
Copy link

Motivation and Context

The wgc CLI currently does not generate a unique identifier for each request made to the control plane. The goal is to generate one when executing a command, propagate it to the control plane (on all requests sent to it), and attach it to control plane logs for easier debugging.

In the case of a control plane error during the execution, the request ID is printed to the user.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.

  • wgc
    • Generated the UUID using node:crypto and attaching to the global config once. This results in the same id being reused on all requests sent to the control panel by a command
    • Added support for the printing of the request ID on errors with a Client interceptor (requestIdInterceptor), so the solution is reused in all subcommands
  • Control panel
    • Modified enrichLogger to extract the request ID header (if it exists) and include it as additional log fields

@turnabout turnabout marked this pull request as ready for review January 10, 2025 03:37
@turnabout turnabout marked this pull request as draft January 10, 2025 03:37
controlplane/src/core/util.ts Outdated Show resolved Hide resolved
cli/src/core/config.ts Outdated Show resolved Hide resolved
cli/src/core/client/client.ts Outdated Show resolved Hide resolved
cli/src/core/client/client.ts Outdated Show resolved Hide resolved
cli/test/get-base-headers.test.ts Outdated Show resolved Hide resolved
cli/test/request-id-interceptor.test.ts Outdated Show resolved Hide resolved
cli/test/request-id-interceptor.test.ts Outdated Show resolved Hide resolved
controlplane/src/core/util.ts Outdated Show resolved Hide resolved
cli/test/request-header.test.ts Outdated Show resolved Hide resolved
@turnabout turnabout marked this pull request as ready for review January 13, 2025 14:40
}
};

await requestIdInterceptor(() => ERR_RESPONSE)(req);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need mock tests, if we can test success and error case with a service implementation here? I'd always suggest real integration tests over spys or mocks. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

I understand your point. The way I see it, this test is testing the interceptor in isolation (as much as possible) while the other is testing the complete feature. I usually like having both of these kinds of tests together, to give me the peace of mind an implementation works as expected by itself, but also in the context of a complete feature.

But I guess there is significant overlap here. If you think they're not needed or not aligned with the rest of the project, I can certainly remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. I do a see a benefit of combining them. Please do 😊

Copy link
Author

Choose a reason for hiding this comment

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

I removed the test file request-id-interceptor.test.ts and ported the two test cases that didn't overlap between them to request-header.test.ts (I assumed that's what you meant by "combining them"). The test cases are:

  • 'Should log the request ID when the response errored'
  • 'Should NOT log the request ID when the response was successful'

Removes `request-id-interceptor.test.ts` and ports test cases that dont'
overlap to `request-header.test.ts`. Also updates `requestIdInterceptor`
to output error message using `console.error`, to differentiate from
other command `console.log` calls during tests
@github-actions github-actions bot removed the router label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants