Skip to content

Add block import tracer provider to plugin-api #8534

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

Merged

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Apr 9, 2025

PR description

Adds the ability to specify a default tracer to AbstractBlockProcessor, so imported blocks may implement tracing.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • spotless: ./gradlew spotlessApply
  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@garyschulte garyschulte marked this pull request as ready for review April 9, 2025 18:07
@garyschulte garyschulte changed the title Add block import tracer provider to plugin-apui Add block import tracer provider to plugin-api Apr 9, 2025
@garyschulte garyschulte force-pushed the feature/block-import-trace-service branch 2 times, most recently from 026f2e5 to 10ca621 Compare April 9, 2025 23:49
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, please add a CHANGELOG entry

Comment on lines 105 to 110
// if we have a BlockImportTracerProvider from pluginContext, use it.
return Optional.ofNullable(pluginContext)
.flatMap(serviceManager -> serviceManager.getService(BlockImportTracerProvider.class))
.map(z -> z.getBlockImportTracer(header))
// otherwise return NO_TRACING
.orElse(BlockAwareOperationTracer.NO_TRACING);
Copy link
Contributor

Choose a reason for hiding this comment

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

not blocking: can the resolution of the tracer to use be done only once since I assume the tracer will not change?

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 wanted to do this in the constructor, but we are not providing a protocol context in the constructor. The lookup is pretty cheap, but I will look again to see how much plumbing it would be to require a protocol context in the constructor

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, if it is not straight forward it could stay like that or just use a memoizing supplier

Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

CHANGELOG entry missing

Comment on lines 105 to 110
// if we have a BlockImportTracerProvider from pluginContext, use it.
return Optional.ofNullable(pluginContext)
.flatMap(serviceManager -> serviceManager.getService(BlockImportTracerProvider.class))
.map(z -> z.getBlockImportTracer(header))
// otherwise return NO_TRACING
.orElse(BlockAwareOperationTracer.NO_TRACING);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, if it is not straight forward it could stay like that or just use a memoizing supplier

@garyschulte
Copy link
Contributor Author

CHANGELOG entry missing

Ah yes, a memoizing supplier would be 👍

@garyschulte garyschulte force-pushed the feature/block-import-trace-service branch from 10ca621 to 0e220bd Compare April 10, 2025 19:20
@garyschulte garyschulte enabled auto-merge (squash) April 10, 2025 19:28
@garyschulte garyschulte merged commit a3cb736 into hyperledger:main Apr 10, 2025
43 checks passed
@garyschulte garyschulte deleted the feature/block-import-trace-service branch April 10, 2025 20:06
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