-
Notifications
You must be signed in to change notification settings - Fork 49
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
Adds ability to change logging verbosity and adds more debug telemetry #594
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5aee87b
to
159f51b
Compare
…dds debug logging statements for upstream requests Signed-off-by: Alex Creasy <[email protected]>
159f51b
to
9ce75f1
Compare
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.
Ok, so far it looks good, and when enabling the app we got log levels, I have a nit in for other build files and other question:
Should we get the logger in the k8s client too? Apologies if I don't fully understand bff still, but the http.go will cover al the MR and swagger routes, but what happens with k8s?
@@ -47,7 +48,7 @@ build: fmt vet test ## Builds the project to produce a binary executable. | |||
.PHONY: run | |||
run: fmt vet envtest ## Runs the project. | |||
ENVTEST_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" \ | |||
go run ./cmd/main.go --port=$(PORT) --mock-k8s-client=$(MOCK_K8S_CLIENT) --mock-mr-client=$(MOCK_MR_CLIENT) --dev-mode=$(DEV_MODE) --dev-mode-port=$(DEV_MODE_PORT) | |||
go run ./cmd/main.go --port=$(PORT) --mock-k8s-client=$(MOCK_K8S_CLIENT) --mock-mr-client=$(MOCK_MR_CLIENT) --dev-mode=$(DEV_MODE) --dev-mode-port=$(DEV_MODE_PORT) --log-level=$(LOG_LEVEL) |
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.
Shouldn't we change as well the dockerfile or other manifest files to reflect the log level we want?
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.
it defaults to INFO which is probably the level we want deployed so we don't slow things down... You can just set an env var on the docker container to change the log level, I should probably alter this so it's more explicit and also add some documentation.
Yeah, I did think about this - I agree it would be really useful to have the k8s stuff logged too. The hard part is right now there's only a single, global k8s client, I guess if I can pass the trace id through somehow, we will have a way of matching things up to a specific "transaction". I'll take a look this afternoon, but will just have a quick chat with Eder first to see how the k8s client is likely to change. |
I should probably add some documentation too!!! s/probably/definitely/ |
Description
Adds several logging related features:
Note: at some point in the future it would be good to add headers to the logging, however we have to be careful about logging confidential credentials etc, so I've intentionally left that part out for now.
How Has This Been Tested?
Try this out with a real MR backend deployed to kind and a mock k8s client, e.g.
make run PORT=4000 MOCK_K8S_CLIENT=true MOCK_MR_CLIENT=false LOG_LEVEL=debug
Make some GET / POST / PATCH requests (the ones from the README are fine)
Verify you get the debug logging.
Also verify different log levels work, e.g. try info and verify the debug messages aren't there. Also try running without the LOG_LEVEL param to ensure it defaults to INFO.
Merge criteria:
DCO
check)If you have UI changes