Skip to content

[DON'T MERGE] Various observability improvement #17491

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
Draft
wants to merge 3 commits into
base: compatible
Choose a base branch
from
Draft

[DON'T MERGE] Various observability improvement #17491

wants to merge 3 commits into from

Conversation

glyh
Copy link
Member

@glyh glyh commented Jul 8, 2025 •
edited
Loading

Status: Danny worried it would make nodes very prone to DDOS. we are considering improve the logging utilities and then merge this.

@glyh glyh requested a review from a team as a code owner July 8, 2025 05:51
@glyh glyh added enhancement Not big enough to be a feature, but is a smaller improvement observability labels Jul 8, 2025
@glyh
Copy link
Member Author

glyh commented Jul 8, 2025

!ci-build-me

@glyh glyh changed the title Mina Run: Log what graphql request is received by daemon Various observability improvement Jul 8, 2025
@dannywillems
Copy link
Member

!ci-build-me

@dannywillems
Copy link
Member

!ci-bypass-changelog

glyh
glyh commented Jul 8, 2025
View reviewed changes
src/app/cli/src/init/mina_run.ml
@@ -531,6 +531,14 @@ let setup_local_server ?(client_trustlist = []) ?rest_server_port
~metadata:
[ ("uri", `String (Uri.to_string uri))
; ("context", `String "rest_server")
; ( "request"
, `String
( Request.sexp_of_t req
Copy link
Member Author

Choose a reason for hiding this comment

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

these cohttp type can't be convert to json, so unfortunate

dannywillems
dannywillems reviewed Jul 8, 2025
View reviewed changes
src/app/cli/src/init/mina_run.ml Outdated
@@ -531,6 +531,14 @@ let setup_local_server ?(client_trustlist = []) ?rest_server_port
~metadata:
[ ("uri", `String (Uri.to_string uri))
; ("context", `String "rest_server")
; ( "request"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it is a fun way to DDOS as we could send large requests and force the log (which require some computations).

Copy link
Member

Choose a reason for hiding this comment

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

I approved, despite this ^.

Copy link
Member Author

@glyh glyh Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well, that could happen before. This PR make it more severe.

I think maybe we can have build disabled those debug logs? Like maybe it could happen at ppx level.

Copy link
Member

@dannywillems dannywillems Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think maybe we can have build disabled those debug logs?

Yes, we could think about that later.

dannywillems
dannywillems approved these changes Jul 8, 2025
View reviewed changes
Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

LGTM.

dannywillems
dannywillems reviewed Jul 8, 2025
View reviewed changes
src/app/cli/src/init/mina_run.ml Outdated
@@ -531,6 +531,14 @@ let setup_local_server ?(client_trustlist = []) ?rest_server_port
~metadata:
[ ("uri", `String (Uri.to_string uri))
; ("context", `String "rest_server")
; ( "request"
Copy link
Member

Choose a reason for hiding this comment

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

This could be moved to a spam or a log of a lower level than debug.

@glyh glyh changed the title Various observability improvement [DON'T MERGE] Various observability improvement Jul 8, 2025
@glyh
Copy link
Member Author

glyh commented Jul 8, 2025

I've no idea what's happening, here's a graphql log entry

{"timestamp":"2025-07-08 08:02:27.599833Z","level":"Debug","source":{"module":"Init__Mina_run","location":"File \"src/app/cli/src/init/mina_run.ml\", line 457, characters 14-26"},"message":"Received graphql request. Uri: $uri","metadata":{"body":"(Pipe((id 230)(buffer())(size_budget 0)(pushback(Full()))(num_values_read 0)(blocked_flushes())(blocked_reads())(closed Empty)(read_closed Empty)(consumers())(upstream_flusheds())))","context":"rest_server","host":"0.0.0.0","mina_run":"Setting up server logs","peer_id":"12D3KooWHSQTZcgHggdteQkJaDNcik1FM91vu71buRDJUemGrhQk","port":10101,"request":"((headers((Accept application/json)(Content-Type application/json)(host 127.0.0.1:10018)(user-agent ocaml-cohttp/5.0.0)(content-length 141)))(meth POST)(scheme())(resource /graphql)(version HTTP_1_1)(encoding(Fixed 141)))","uri":"//127.0.0.1:10018/graphql"}}

the sexp doesn't contain anything meaningful.

@dannywillems
Copy link
Member

Draft for now.

@dannywillems dannywillems marked this pull request as draft July 8, 2025 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannywillems dannywillems dannywillems approved these changes

Assignees
No one assigned
Labels
enhancement Not big enough to be a feature, but is a smaller improvement observability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@glyh @dannywillems