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

[tracing] Add tracing for Garbage Collection #10177

Merged

Conversation

shreyan-gupta
Copy link
Contributor

@shreyan-gupta shreyan-gupta commented Nov 15, 2023

Adding logs for generic GC.
Tracing for GC during the last block of the epoch during which resharding happened.

@shreyan-gupta shreyan-gupta requested a review from a team as a code owner November 15, 2023 09:41
@@ -2344,6 +2344,7 @@ impl<'a> ChainStoreUpdate<'a> {
// Now we can proceed to removing the trie state and flat state
let mut store_update = self.store().store_update();
for shard_uid in prev_shard_layout.get_shard_uids() {
tracing::info!("GC resharding, block_hash {}, shard_uid {}", block_hash, shard_uid);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check out the style guidelines, on tracing use at https://github.com/near/nearcore/blob/master/docs/practices/style.md#Tracing.

Suggested change
tracing::info!("GC resharding, block_hash {}, shard_uid {}", block_hash, shard_uid);
tracing::info!(target: "...", block_hash, shard_uid, "GC resharding");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies

@shreyan-gupta shreyan-gupta changed the title [resharding] Add tracing for GC after resharding [tracing] Add tracing for Garbage Collection Nov 15, 2023
@@ -996,7 +996,7 @@ impl Chain {
tries: ShardTries,
gc_config: &near_chain_configs::GCConfig,
) -> Result<(), Error> {
let _span = tracing::debug_span!(target: "chain", "clear_data").entered();
let _span = tracing::debug_span!(target: "garbage_collection", "clear_data").entered();
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the target here seems a bit controversial, what is your goal here?
Personally I would rather keep it as chain but add the GC prefix as you've done elsewhere. Not a blocker, more of a preference and I don't think we have any guidelines on this so feel free to keep as is.

Comment on lines +80 to +82
GCMode::Fork(_) => write!(f, "GCMode::Fork"),
GCMode::Canonical(_) => write!(f, "GCMode::Canonical"),
GCMode::StateSync { .. } => write!(f, "GCMode::StateSync"),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You can use the f.debug_struct method. Under the hood it'll also use write but you're guaranteed that its style will be consistent with the rest of the codebase and the defaults. You can find plenty of examples in the codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked, I think write! is fine here as we don't have any other fields we would like to print out here.

@shreyan-gupta shreyan-gupta added this pull request to the merge queue Nov 15, 2023
@shreyan-gupta shreyan-gupta removed this pull request from the merge queue due to a manual request Nov 15, 2023
@shreyan-gupta shreyan-gupta added this pull request to the merge queue Nov 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2023
@shreyan-gupta shreyan-gupta added this pull request to the merge queue Nov 15, 2023
Merged via the queue into near:master with commit e7f879f Nov 15, 2023
15 of 16 checks passed
@shreyan-gupta shreyan-gupta deleted the shreyan/resharding/gc_tracing branch November 15, 2023 13:54
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.

3 participants