Skip to content

chore: demote session established log level#4564

Merged
mattsse merged 2 commits intomainfrom
rkrasiuk/demote-session-established-log
Sep 18, 2023
Merged

chore: demote session established log level#4564
mattsse merged 2 commits intomainfrom
rkrasiuk/demote-session-established-log

Conversation

@rkrasiuk
Copy link
Contributor

@rkrasiuk rkrasiuk commented Sep 12, 2023

Description

Demote the log level of "Session Established" to debug

@rkrasiuk rkrasiuk added the A-observability Related to tracing, metrics, logs and other observability tools label Sep 12, 2023
@rkrasiuk rkrasiuk requested a review from rakita September 12, 2023 09:34
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather go for debug level, because these logs are not as frequent as other network-related logs we already have on trace level in other crates.

Copy link
Member

Choose a reason for hiding this comment

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

Also maybe combine it with the next debug log Established peer enode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

debatable because of the intention of the log: debug as the name suggests is mostly for debugging, trace is for more detailed information about a particular action of the application

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merged the two, pending @mattsse

Copy link
Member

Choose a reason for hiding this comment

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

In my head trace was always about even more hardcore debugging than debug, and mostly useful for core devs who want to step through the function behavior using these trace logs. While debug is also for regular users, who would like to know that, e.g. in the case, they established a connection with some peer but without too much implementation-specific details.

Copy link
Collaborator

Choose a reason for hiding this comment

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

both trace and debug are for debugging, imo we abuse trace a little and do not use debug when needed. I usually use debug when it can be read from the console but we don't want to have it in a normal flow, and trace if it is something that can only be read if it is in file (log too big or it is frequent (spammy)).

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #4564 (43e49dd) into main (25558b3) will decrease coverage by 0.71%.
Report is 29 commits behind head on main.
The diff coverage is 25.00%.

Impacted file tree graph

Files Changed Coverage Δ
crates/net/network/src/manager.rs 47.75% <25.00%> (-0.26%) ⬇️

... and 107 files with indirect coverage changes

Flag Coverage Δ
integration-tests 16.99% <25.00%> (+0.35%) ⬆️
unit-tests 63.16% <25.00%> (-0.82%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 31.37% <ø> (+0.53%) ⬆️
blockchain tree 83.59% <ø> (+0.01%) ⬆️
pipeline 88.54% <ø> (-2.00%) ⬇️
storage (db) 73.44% <ø> (-1.89%) ⬇️
trie 94.73% <ø> (-0.15%) ⬇️
txpool 49.16% <ø> (+1.36%) ⬆️
networking 77.22% <25.00%> (-0.06%) ⬇️
rpc 57.34% <ø> (-0.02%) ⬇️
consensus 62.42% <ø> (-0.99%) ⬇️
revm 19.67% <ø> (-11.89%) ⬇️
payload builder 9.06% <ø> (+2.71%) ⬆️
primitives 86.42% <ø> (-0.12%) ⬇️

@rkrasiuk rkrasiuk force-pushed the rkrasiuk/demote-session-established-log branch from 00ee096 to 8215a66 Compare September 12, 2023 09:51
);
debug!(target: "net", kind=%direction, peer_enode=%NodeRecord::new(remote_addr, peer_id), "Established peer enode");
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on adding the enode back? it was useful when investigating suspicious peers

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

added the enode to the log

@mattsse mattsse enabled auto-merge September 18, 2023 08:29
@mattsse mattsse added this pull request to the merge queue Sep 18, 2023
Merged via the queue into main with commit 8baf234 Sep 18, 2023
@mattsse mattsse deleted the rkrasiuk/demote-session-established-log branch September 18, 2023 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-observability Related to tracing, metrics, logs and other observability tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants