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 crash bug related to sharding #455

Merged
merged 2 commits into from
May 16, 2022
Merged

Fix crash bug related to sharding #455

merged 2 commits into from
May 16, 2022

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented May 16, 2022

Which problem is this PR solving?

  • Fixes a bug where the shard index can get out of range if the last 4 bytes of the hash of the trace ID is close to 0xFFFFFFFF and the number of shards doesn't divide that value evenly. The algorithm in WhichShard works correctly for divisors of 2^32-1. The prime factorization of that includes 1, 3, 5, 17 (and products of those).

Short description of the changes

  • If shard would be out of range, assigns it to shard 0. This has the most minimal effect on existing behavior, especially when the number of shards changes.
  • Adds a test that failed before the out of range check, but works with it in.

Fixes #454

@kentquirk kentquirk requested a review from a team May 16, 2022 14:59
@robbkidd robbkidd added version: bump patch A PR with release-worthy changes and is backwards-compatible. type: bug Something isn't working labels May 16, 2022
@kentquirk kentquirk force-pushed the kent.trace_sharding branch from 3089ca7 to d1a8cfa Compare May 16, 2022 19:18
Copy link
Member

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

Back to the start!

Annny chance of a test case?

@kentquirk
Copy link
Contributor Author

That sounds like a fine idea. Will do!

@kentquirk kentquirk merged commit 3558a28 into main May 16, 2022
@kentquirk kentquirk deleted the kent.trace_sharding branch May 16, 2022 22:37
kentquirk added a commit that referenced this pull request May 17, 2022
## 1.14.1 2022-05-16

### Fixes

- Fix crash bug related to sharding (#455) | [@kentquirk](https://github.com/kentquirk)

### Maintenance

- bump husky to 0.10.5 (#450) | [@MikeGoldsmith](https://github.com/MikeGoldsmith)
- Bump github.com/klauspost/compress from 1.15.2 to 1.15.4 (#451) | dependabot
- Bump github.com/tidwall/gjson from 1.14.0 to 1.14.1 (#444) | dependabot
- Bump github.com/fsnotify/fsnotify from 1.5.1 to 1.5.4 (#441) | dependabot

### Documentation

- add a note about reloading the configuration when running within docker (#448) | [@leviwilson](https://github.com/leviwilson)
- README: remove incorrect mention of sending SIGUSR1 to trigger a configuration reload (#447) | [@jharley](https://github.com/jharley)
ghost pushed a commit to opsramp/tracing-proxy that referenced this pull request Jul 5, 2024
	// The algorithm in WhichShard works correctly for divisors of 2^32-1. The prime factorization of that includes
	// 1, 3, 5, 17, so we need something other than 3 to be sure that this test would fail.
	// It was tested (and failed) without the additional conditional.
ghost pushed a commit to opsramp/tracing-proxy that referenced this pull request Jul 5, 2024
## 1.14.1 2022-05-16

### Fixes

- Fix crash bug related to sharding (honeycombio#455) | [@kentquirk](https://github.com/kentquirk)

### Maintenance

- bump husky to 0.10.5 (honeycombio#450) | [@MikeGoldsmith](https://github.com/MikeGoldsmith)
- Bump github.com/klauspost/compress from 1.15.2 to 1.15.4 (honeycombio#451) | dependabot
- Bump github.com/tidwall/gjson from 1.14.0 to 1.14.1 (honeycombio#444) | dependabot
- Bump github.com/fsnotify/fsnotify from 1.5.1 to 1.5.4 (honeycombio#441) | dependabot

### Documentation

- add a note about reloading the configuration when running within docker (honeycombio#448) | [@leviwilson](https://github.com/leviwilson)
- README: remove incorrect mention of sending SIGUSR1 to trigger a configuration reload (honeycombio#447) | [@jharley](https://github.com/jharley)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working version: bump patch A PR with release-worthy changes and is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sharding bug can crash refinery
2 participants