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

add tracing metrics layer for dependency logging #4979

Merged
merged 19 commits into from
Jan 16, 2024

Conversation

divagant-martian
Copy link
Collaborator

@divagant-martian divagant-martian commented Dec 4, 2023

Issue Addressed

discv5#213 and partially #4954

Proposed Changes

add a tracing subscriber that registers metrics for dependencies

Additional Info

discv5 logs warnings like craaazy 💀

sample output from the first seconds of running lh

# HELP dep_info_total Count of infos logged per enabled dependency
# TYPE dep_info_total counter
dep_info_total{target="discv5"} 4
# HELP dep_warn_total Count of warns logged per enabled dependency
# TYPE dep_warn_total counter
dep_warn_total{target="discv5"} 22
dep_warn_total{target="libp2p_gossipsub"} 1
dep_warn_total{target="quinn_udp"} 1

@divagant-martian divagant-martian changed the title add metrics layer add tracing metrics layer for dependency logging Dec 4, 2023
@divagant-martian divagant-martian marked this pull request as ready for review December 5, 2023 16:09
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Nice! Its great this wasn't such a pain.

One small comment.

I think we should enable this by default for WARN's and above. Am I right in understanding we wont get any metrics by default?

In a future PR we will remove the -l switch all-together and just default log to files. So I think would be good to by default just log warn's and aboves in the metrics.

@AgeManning
Copy link
Member

To be clear.

I think in this PR we make a breaking change and remove the -l flag. We need logs in order to have these metrics but we dont want to add the logs to the terminal. Doing so makes the terminal a giant mess.

For the time being, this will destroy the ability to look at dependency logs. Until we have #4954 which should then log the dependency logs to individual files.

I think we opt for useful metrics now and then file logging later. This should all be by default and the -l flag becomes deprecated.

@divagant-martian
Copy link
Collaborator Author

divagant-martian commented Dec 6, 2023

@AgeManning I gave a (honestly quick) look into having logs enabled but output disabled and configuring the writes is something I think belongs more in #4954 than here, it's otherwise a bit of lost work, plus defeats the purpose in some way until that work is completed.

For instance, I do need to able to look into the logs from discv5 in the meantime to check what we are logging as warns to either expand the logged info, or downgrade the logs. Clearing the output prevents me to meaningfully see what's going on in discv5 in the context of LH until file logging is addressed.

I pushed a middle ground commit that goes with your original suggestion:

I think we should enable this by default for WARN's and above. Am I right in understanding we wont get any metrics by default?

In a future PR we will remove the -l switch all-together and just default log to files. So I think would be good to by default just log warn's and aboves in the metrics

let logger = Logger::root(drain.fuse(), o!());
let _scope_guard = slog_scope::set_global_logger(logger);
slog_stdlog::init_with_level(debug_level).unwrap();
let log = Logger::root(drain.fuse(), o!());
Copy link
Collaborator Author

@divagant-martian divagant-martian Dec 6, 2023

Choose a reason for hiding this comment

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

@AgeManning had to remove this to make the bootnode logging not panic. It seems to work alright, any idea why the rest of the lines were here?

Copy link
Member

Choose a reason for hiding this comment

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

I think they needed to be there in order to get the logs out to display inside slog.

If you run a boot node, do we still get logs to the terminal?

Copy link
Collaborator Author

@divagant-martian divagant-martian Dec 12, 2023

Choose a reason for hiding this comment

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

yeah I get them alright. Sample:

Dec 12 23:23:45.909 INFO Executing bootstrap query...
2023-12-12T23:23:45.909324Z  INFO discv5::service: Discv5 Service started
2023-12-12T23:23:45.909386Z  INFO discv5::service: Ip4
Dec 12 23:23:50.126 INFO Server metrics                          unreachable_nodes: 0, ipv6_and_ipv4_nodes: 0, ipv6_nodes: 0, ipv4_nodes: 18, requests/s: 0.00, active_sessions: 33, connected_peers: 18

first and last are slog ones, middle ones are tracing ones

@divagant-martian divagant-martian added ready-for-review The code is ready for review Networking labels Dec 11, 2023
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Looks good to me. Have reviewed also via #4988

@divagant-martian
Copy link
Collaborator Author

divagant-martian commented Dec 12, 2023

@AgeManning please note #4988 needs a rebase, not all changes here are there. Can we merge this one in preparation for that one? I don't think 4988 is completely ready yet but would prefer to have them merged as two prs and not one

@divagant-martian divagant-martian added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Dec 18, 2023
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

LGTM

@AgeManning AgeManning merged commit a68b701 into sigp:unstable Jan 16, 2024
13 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants