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.

Sign up for GitHub

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

Jump to bottom

Remove Tracer packet as a concept #4043

Merged
merged 9 commits into from
Dec 12, 2024
Merged

Remove Tracer packet as a concept #4043

merged 9 commits into from
Dec 12, 2024

Conversation

apfitzge
Copy link

Problem

  • I quietly rugged the metrics reported by banking stage for tracer packets
  • No one complained...which means no one uses these

Summary of Changes

  • Remove Tracer Packet concept at all levels

Fixes #

@apfitzge apfitzge force-pushed the remove_tracer_stats branch from b8cffd2 to 838bbc9 Compare December 10, 2024 17:26
@apfitzge apfitzge marked this pull request as ready for review December 10, 2024 18:39
@apfitzge
Copy link
Author

apfitzge commented Dec 10, 2024 •
edited
Loading

@ryoqun could affect you - this will break backward compatibility with banking-trace files.

@ryoqun
Copy link
Member

ryoqun commented Dec 11, 2024

@ryoqun could affect you - this will break backward compatibility with banking-trace files.

I'm okay with this new abi incompatibility.

@apfitzge
Copy link
Author

I'm okay with this new abi incompatibility.

Realizing I should probably also add a note to the change log about this.
Best if operators delete/move their old banking-trace when upgrading to version supporting this, because otherwise the files will be a mixture of both. If they don't would just make the files useless for the few days/hours it takes to roll over.

bw-solana
bw-solana reviewed Dec 11, 2024
View reviewed changes
Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

LGTM overall. Couple minor things to consider

core/src/sigverify.rs Outdated Show resolved Hide resolved
sdk/packet/src/lib.rs Show resolved Hide resolved
@apfitzge apfitzge force-pushed the remove_tracer_stats branch from 838bbc9 to 3310708 Compare December 11, 2024 18:33
@apfitzge apfitzge self-assigned this Dec 11, 2024
@apfitzge apfitzge force-pushed the remove_tracer_stats branch from 9a23f07 to 240f07d Compare December 12, 2024 21:01
bw-solana
bw-solana approved these changes Dec 12, 2024
View reviewed changes
Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

LGTM

@apfitzge apfitzge merged commit 3a6d593 into anza-xyz:master Dec 12, 2024
50 checks passed
@ryoqun ryoqun mentioned this pull request Dec 16, 2024
Draft
8 tasks
@apfitzge apfitzge mentioned this pull request Dec 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bw-solana bw-solana bw-solana approved these changes

@tao-stones tao-stones Awaiting requested review from tao-stones

Assignees

@apfitzge apfitzge

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apfitzge @ryoqun @bw-solana