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

Improve logging #87

Merged
merged 2 commits into from
Jun 19, 2024
Merged

Improve logging #87

merged 2 commits into from
Jun 19, 2024

Conversation

stormshield-gt
Copy link
Collaborator

Attempt to address some of the concerns raised in #86

  • add the HTTP verb to the log to differentiate which endpoint is actually used
  • use structured logging, so it will be easier to process the log afterward. For instance, structured logging allows data to be a key value inside a json
  • move the span from the top-level function to the inner wrapper, so we don't get to see the parameters. Parameters can contain sensitive information (prevent leaking unseal key #85) and tends to bloat the logs. Users can still log the parameter if they want by adding a trace themself.
  • remove some info traces that were redundant or did not contains useful information

Here there is an example to the new log, which can be useful to compare with the one in #85 . Here, RUST_LOG="info,rustify=off" is used, as it won't likely to be fixed upstream.

Unhappy path:

2024-03-27T08:06:02.413716Z  INFO request{method=GET path=/sys/health}: vaultrs::api: start request
2024-03-27T08:06:02.422478Z ERROR request{method=GET path=/sys/health}: vaultrs::api: error=An error occurred with the request

Happy path:

2024-03-27T08:06:00.283379Z  INFO request{method=POST path=/sys/init}: vaultrs::api: start request

@Haennetz
Copy link
Collaborator

Can you please rebase your branch witch main to resolve the conflict?

@stormshield-gt
Copy link
Collaborator Author

This should be fine now

@Haennetz
Copy link
Collaborator

Nice work.

@Haennetz Haennetz merged commit 3e911f2 into jmgilman:master Jun 19, 2024
7 checks passed
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