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

fix: apply log_summary_style to state sync phase message #8735

Merged
merged 8 commits into from
Mar 21, 2023

Conversation

nikurt
Copy link
Contributor

@nikurt nikurt commented Mar 15, 2023

No description provided.

@nikurt nikurt requested a review from a team as a code owner March 15, 2023 12:29
@nikurt nikurt requested a review from mzhangmzz March 15, 2023 12:29
@@ -176,6 +177,7 @@ impl StateSync {
now: DateTime<Utc>,
state_parts_task_scheduler: &dyn Fn(ApplyStatePartsRequest),
state_split_scheduler: &dyn Fn(StateSplitRequest),
use_colour: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhat unfortunate that we must thread this through like that…

}
_ => unreachable!("timeout cannot happen when all state is downloaded"),
},
phase = format_shard_sync_phase(&shard_sync_download, use_colour),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this would be better served by splitting the phase up into different tracing KVs? for example state_requests_count and last_target could both easily be KVs I think 🤔

Though adding colors to that would be even harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm worried about the StateDownloadParts branch. Some shards have a lot of shards, which results in huge log messages. I definitely don't want to export 50k tracing KVs from a single span.
At the moment, this way-too-verbose logging is still useful for development of State Sync. Currently, I'm leaning to refactoring the code and keeping the logging.

I'll consider simplifying the log line in the future. Coutning the number of done/not-done parts should be useful enough in most cases.

@near-bulldozer near-bulldozer bot merged commit db103ad into near:master Mar 21, 2023
nikurt added a commit to nikurt/nearcore that referenced this pull request Mar 23, 2023
* Use colour for state sync phase display

* fix

* fix

* debug verbosity

* fix
nikurt added a commit to nikurt/nearcore that referenced this pull request Apr 6, 2023
* Use colour for state sync phase display

* fix

* fix

* debug verbosity

* fix
nikurt added a commit to nikurt/nearcore that referenced this pull request Apr 13, 2023
* Use colour for state sync phase display

* fix

* fix

* debug verbosity

* fix
nikurt added a commit that referenced this pull request Apr 14, 2023
* Use colour for state sync phase display

* fix

* fix

* debug verbosity

* fix
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.

2 participants