Skip to content

[v0.47] [Util] Improve remote debugging tooling#8504

Open
turbolent wants to merge 15 commits intov0.47from
bastian/v0.47-debugging-improvements
Open

[v0.47] [Util] Improve remote debugging tooling#8504
turbolent wants to merge 15 commits intov0.47from
bastian/v0.47-debugging-improvements

Conversation

@turbolent
Copy link
Copy Markdown
Member

Port #8440 to v0.47, so we can compare v0.47 against v0.48

@turbolent turbolent requested a review from a team as a code owner March 24, 2026 19:41
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 24, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f360b27c-237b-4f3b-aa9c-828625b41380

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bastian/v0.47-debugging-improvements

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 24, 2026

Comment on lines +95 to +101
span := programs.tracer.StartChildSpan(trace.FVMEnvGetOrLoadProgram)
if span.Tracer != nil {
span.SetAttributes(
attribute.String("location", location.ID()),
)
}
defer span.End()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would prefer to avoid having the if span.Tracer != nil check if possible.

We can add a SetAttributes to tracing.TracerSpan:

func (tracer TracerSpan) SetAttributes(kv ...attribute.KeyValue) {
	if tracer.IsTraceable() {
		tracer.Span.SetAttributes(kv...)
	}
}

And I think this instance can even be replaced with:

defer programs.tracer.StartChildSpan(
		trace.FVMEnvGetOrLoadProgram,
		otel.WithAttributes(attribute.String("location", location.ID())),
	).End()


Cmd.Flags().Uint64Var(&flagComputeLimit, "compute-limit", flow.DefaultMaxTransactionGasLimit, "transaction compute limit")

Cmd.Flags().BoolVar(&flagUseExecutionDataAPI, "use-execution-data-api", true, "use the execution data API (default: true)")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Cmd.Flags().BoolVar(&flagUseExecutionDataAPI, "use-execution-data-api", true, "use the execution data API (default: true)")
Cmd.Flags().BoolVar(&flagUseExecutionDataAPI, "use-execution-data-api", true, "use the execution data API")

cobra already states the defaults


// resolveBlockChain fetches count consecutive block IDs starting from startBlockID,
// following parent IDs, and returns them as hex strings.
func resolveBlockChain(startBlockID string, count int) []string {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we are all trying to resolve the blockchain in one way or another...
Maybe a better naming would be getBlocksAfter.

Also, if I'm not mistaken, the method gets blocks before startBlockID, since it goes to the parent. Is that intentional? If it is, it is a bit counterintuitive and should definitely be documented.

@janezpodhostnik janezpodhostnik requested a review from a team March 26, 2026 12:22
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