Skip to content

chore: remove the ProxyInner trait #6

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

chore: remove the ProxyInner trait #6

wants to merge 4 commits into from
+181 −177

Conversation

@akundaz
Copy link
Contributor

@akundaz akundaz commented Oct 22, 2025

This trait doesn't do anything except for getting the proxy name for logging and metrics. There are simpler ways to do this and this PR uses tracing spans and simply passes the name as an argument as necessary

@akundaz akundaz self-assigned this Oct 22, 2025
0x416e746f6e
0x416e746f6e reviewed Oct 22, 2025
View reviewed changes
0x416e746f6e
0x416e746f6e reviewed Oct 22, 2025
View reviewed changes
0x416e746f6e
0x416e746f6e reviewed Oct 22, 2025
View reviewed changes
crates/rproxy/src/server/proxy/http/rpc.rs
{
Ok(jrpc_response) => jrpc_response,
Err(err) => {
warn!(error = ?err, "Failed to parse json-rpc response");
Copy link
Member

@0x416e746f6e 0x416e746f6e Oct 22, 2025

Choose a reason for hiding this comment

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

issue:

proxy field shouldn't be removed. it helps when trouble-shooting to focus on just one kind of a proxy, and this field helps with that.

Copy link
Member

@0x416e746f6e 0x416e746f6e Oct 23, 2025

Choose a reason for hiding this comment

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

proxy field is still missing in the log emitted by this line

@0x416e746f6e
Copy link
Member

0x416e746f6e commented Oct 22, 2025

This trait doesn't do anything except for getting the proxy name for logging and metrics. There are simpler ways to do this and this PR uses tracing spans and simply passes the name as an argument as necessary

there is a bunch of places where proxy field is being removed from the logs. I didn't comment on all of them, but please revert those changes.

at some point with bproxy I was literally using that field on a daily basis to trouble-shoot. I will need it with rproxy just the same.

@akundaz
Copy link
Contributor Author

akundaz commented Oct 22, 2025

there is a bunch of places where proxy field is being removed from the logs. I didn't comment on all of them, but please revert those changes.

The field isn't being removed it was put in the enclosing span. That's what these lines in server.rs are for

.instrument(info_span!("proxy_name", proxy = "........"))

@0x416e746f6e
Copy link
Member

0x416e746f6e commented Oct 22, 2025

there is a bunch of places where proxy field is being removed from the logs. I didn't comment on all of them, but please revert those changes.

The field isn't being removed it was put in the enclosing span. That's what these lines in server.rs are for

.instrument(info_span!("proxy_name", proxy = "........"))

I don's see it in the logs:

{"timestamp":"2025-10-22T04:59:10.652563Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7200-71c2-9b2f-14157de1e134","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652623Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7203-7012-812e-8f61e10e309d","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652587Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7204-74f0-8419-fe9384fb2c13","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652664Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7204-74f0-8419-fe5a5b802afd","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652731Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7205-77c2-afcc-7f0564c8d856","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652735Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7205-77c2-afcc-7ee7219f959a","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652778Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7206-7583-a291-6fa0990538c8","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652563Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7202-7561-b4ce-6253b6131cbe","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652563Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7200-71c2-9b2f-1400016dc126","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652640Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7205-77c2-afcc-7ece19d9b358","target":"rproxy::server::proxy::http::proxy"}
{"timestamp":"2025-10-22T04:59:10.652607Z","level":"DEBUG","message":"Destroying http-proxy worker...","worker_id":"019a0a37-7204-74f0-8419-fe70304374da","target":"rproxy::server::proxy::http::proxy"}

@0x416e746f6e
Copy link
Member

0x416e746f6e commented Oct 22, 2025 •
edited
Loading

also, I would very much like to avoid using instrumentation spans. these are a disaster to navigate, and unless we really use some advanced tracing platform add absolutely no value.

for example:

{
    "timestamp": "2025-10-22T14:18:43.291839Z",
    "level": "INFO",
    "message": "Starting websocket-proxy...",
    "listen_address": "0.0.0.0:1111",
    "workers_count": 10,
    "target": "rproxy::server::proxy::ws::proxy",
    "span": {
        "proxy": "rproxy-flashblocks",
        "name": "proxy_name"
    },
    "spans": [
        {
            "proxy": "rproxy-flashblocks",
            "name": "proxy_name"
        }
    ]
}

^-- I will need to make +2 clicks on the structured log in the UI just to see the proxy name

compare with:

{
    "timestamp": "2025-10-22T14:22:03.907551Z",
    "level": "INFO",
    "message": "Starting websocket-proxy...",
    "proxy": "rproxy-flashblocks",
    "listen_address": "0.0.0.0:1111",
    "workers_count": 10,
    "target": "rproxy::server::proxy::ws::proxy"
}

@akundaz
Copy link
Contributor Author

akundaz commented Oct 22, 2025

I've added the proxy field back to all the log messages. We can resolve the dispute over using spans and a custom formatter later

0x416e746f6e
0x416e746f6e reviewed Oct 23, 2025
View reviewed changes
crates/rproxy/src/server/proxy/http/rpc.rs
{
Ok(jrpc_response) => jrpc_response,
Err(err) => {
warn!(error = ?err, "Failed to parse json-rpc response");
Copy link
Member

@0x416e746f6e 0x416e746f6e Oct 23, 2025

Choose a reason for hiding this comment

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

proxy field is still missing in the log emitted by this line

crates/rproxy/src/server/proxy/http/rpc.rs
Ok(jrpc_response) => jrpc_response,
Err(err) => {
warn!(proxy = Self::name(), error = ?err, "Failed to parse json-rpc response");
warn!(error = ?err, "Failed to parse json-rpc response");
Copy link
Member

@0x416e746f6e 0x416e746f6e Oct 23, 2025

Choose a reason for hiding this comment

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

issue:

proxy field is gone here too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0x416e746f6e 0x416e746f6e 0x416e746f6e left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@akundaz akundaz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@akundaz @0x416e746f6e