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

Refactor Middleware to Logger #845

Merged
merged 14 commits into from
Aug 4, 2022
Merged

Refactor Middleware to Logger #845

merged 14 commits into from
Aug 4, 2022

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Aug 3, 2022

Change the name of the Middleware traits (HTTP / WS) to better reflect
its purposes.

Closes #843.

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Aug 3, 2022
@lexnv lexnv requested a review from a team as a code owner August 3, 2022 15:17
http-server/src/server.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

I'm not certain on the word Metrics on it's own but I could live with it! Logger was another word that came to mind :)

Co-authored-by: James Wilson <james@jsdw.me>
@niklasad1
Copy link
Member

yeah, I'm not a fan of using HttpMetrics either we should really have like HttpRpcLogging/Metrics because we need to make it clear that you can't really do any logging/metrics on the underlying HTTP/WS instead it gives timings on the specific JSON-RPC calls except some edge-cases just as on_connect but perhaps that can be removed with tower middleware...

@jsdw
Copy link
Collaborator

jsdw commented Aug 3, 2022

I mean, since on_connect and on_disconnect exist, we do allow some logging based on the whole http/ws connection too?

My fave really would be HttpLogger or WsLogger; the user would then impl eg HttpLogger for some struct and it would do some sort of logging of http things (mainly the rpc request bit but also the whole http cycle via the on_connect/on_disconnect too right?).

impl HttpMetrics for SomeStruct feels a bit wonky/less clear to me (and, like, we might log metrics or we might log some other stuff, but logging something is really the thing that this trait enables, basically).

@niklasad1
Copy link
Member

niklasad1 commented Aug 3, 2022

with that said, shouldn't we expose the entire HTTP request then instead of just the headers?

then I'm on board with HttpLogger

@jsdw
Copy link
Collaborator

jsdw commented Aug 4, 2022

If it's easy to do then I don't see why not!

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
…3_metrics_refactoring

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv changed the title Refactor Middleware to Metrics Refactor Middleware to Logger Aug 4, 2022
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, nice work

@lexnv lexnv merged commit 21e557d into master Aug 4, 2022
@lexnv lexnv deleted the 843_metrics_refactoring branch August 4, 2022 16:22
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.

Rename HttpMiddleware/WsMiddleware to avoid confusion
3 participants