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

logging enhancements #57

Merged
merged 3 commits into from
Jun 20, 2024
Merged

logging enhancements #57

merged 3 commits into from
Jun 20, 2024

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Jun 14, 2024

What

  • http context logging at debug level.
  • http context id consistent for all loggers with the format #{} at the end of the log line.
  • Fix clippy complaining about rate limit policy name not being used. Added the name to a log line (at debug level).

@eguzki eguzki requested review from alexsnaps and a team June 14, 2024 08:17
@eguzki
Copy link
Contributor Author

eguzki commented Jun 14, 2024

TODO in future PR's: review logging traces again under heavy traffic stress. When multiple requests hit "at the same time", try to see if one single request can be traced for debugging.

Copy link
Member

@adam-cattermole adam-cattermole left a comment

Choose a reason for hiding this comment

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

Changes look good to me. I guess my only question is the reason for moving the context id to the end of the line? I guess it's personal preference? I'm undecided whether it's easier to identify the logs you're looking for with the identifier at the start vs at the end

@adam-cattermole
Copy link
Member

adam-cattermole commented Jun 19, 2024

I guess to add to my thoughts there - if there is going to be logs with different context id's interleaved would it not be easier to debug with the context id at the start instead of having to look at the end of each log first?

@eguzki
Copy link
Contributor Author

eguzki commented Jun 19, 2024

ere is going to be logs with different context

Fair point. Not a strong opinion here. I saw there was some logs doing that and I tried to apply some consistency.

I will add at the start

@eguzki
Copy link
Contributor Author

eguzki commented Jun 20, 2024

For the record: the context id is related to the connection. Multiple (slow) requests share the same context id (tested).

Following same approach as the example from rust-sdk lib https://github.com/proxy-wasm/proxy-wasm-rust-sdk/blob/main/examples/http_headers/src/lib.rs#L46-L48

for (name, value) in &self.get_http_request_headers() {
            info!("#{} -> {}: {}", self.context_id, name, value);
}

Copy link
Contributor

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

🔎

@eguzki eguzki merged commit 3297733 into main Jun 20, 2024
6 checks passed
@eguzki eguzki deleted the logging-enhancements branch June 20, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants