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

Adds ability to change logging verbosity and adds more debug telemetry #594

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexcreasy
Copy link
Contributor

@alexcreasy alexcreasy commented Nov 25, 2024

Description

Adds several logging related features:

  • Ability to change logging verbosity through a cli arg or environment variable
  • Adds a unique trace id to all requests via context to link them together
  • Adds debug logging for upstream requests / responses with a unique request id

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:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.
  • Code changes follow the kubeflow contribution guidelines.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from alexcreasy. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…dds debug logging statements for upstream requests

Signed-off-by: Alex Creasy <[email protected]>
Copy link
Contributor

@lucferbux lucferbux left a 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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@alexcreasy
Copy link
Contributor Author

alexcreasy commented Nov 26, 2024

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.

@alexcreasy
Copy link
Contributor Author

alexcreasy commented Nov 26, 2024

I should probably add some documentation too!!!

s/probably/definitely/

@lucferbux
Copy link
Contributor

/hold

We are gonna wait until #599 is merged.

@lucferbux
Copy link
Contributor

/unhold

@lucferbux
Copy link
Contributor

@alexcreasy I think there's still some changes left but you can rebase this safely

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.

2 participants