fix: make sure decision keep and drop is persisted in redis #1020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In order to persist the the decisionDrop for a given trace, this commit makes sure we are following the same state transition pattern as other state events for DecisionDrop. When a trace is marked as dropped, span information is also deleted.
Utilize SetNXHash to set KeepReason only once. This change also does proper state transition for decisionKeep.
kentquirk
approved these changes
Feb 29, 2024
centralstore/smartwrapper_test.go
Outdated
| @@ -330,7 +331,7 @@ func TestSetTraceStatuses(t *testing.T) { | |||
| if status.TraceID == traceids[0] { | |||
| assert.Equal(t, DecisionKeep, status.State) | |||
| // TODO: save the reason in the reason cache correctly | |||
There was a problem hiding this comment.
remove this TODO, please
internal/redis/redis.go
Outdated
| } | ||
| } | ||
|
|
||
| // TODO: values is always "OK", but we should be able to get the values |
There was a problem hiding this comment.
I don't understand what this TODO is saying?
centralstore/smartwrapper_test.go
Outdated
| @@ -433,14 +434,14 @@ func BenchmarkStoreGetTracesForState(b *testing.B) { | |||
| } | |||
| } | |||
|
|
|||
| func cleanupRedisStore(store BasicStorer) error { | |||
| func cleanupRedisStore(t testing.TB, store BasicStorer) error { | |||
There was a problem hiding this comment.
Given that you're logging and not returning an error, should probably remove the return parameter.
VinozzZ
force-pushed
the
yingrong.save_keep_reason_in_redis
branch
4 times, most recently
from
abb6333 to
aa3df08
Compare
The score in a sorted set is a float64. Nanosecond can't fit into a float64. That's why we are changing it to microsecond.
VinozzZ
force-pushed
the
yingrong.save_keep_reason_in_redis
branch
from
aa3df08 to
9e63c53
Compare
kentquirk
pushed a commit
that referenced
this pull request
Mar 15, 2024
<!-- Thank you for contributing to the project! 💜 Please make sure to: - Chat with us first if this is a big change - Open a new issue (or comment on an existing one) - We want to make sure you don't spend time implementing something we might have to say No to - Add unit tests - Mention any relevant issues in the PR description (e.g. "Fixes #123") Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate. --> ## Which problem is this PR solving? Before this PR, decisions made for traces were not stored in redis. It also wasn't part of the state machine when transition a trace from awaiting for decision to either decision keep or drop. ## Short description of the changes - Introduced DecisionDrop and DecisionKeep into the state machine - check against in-memory cache before fetching from Redis for trace status - run smartwrapper tests against redis in the ci - removed some unnecessary code
kentquirk
pushed a commit
that referenced
this pull request
Mar 22, 2024
<!-- Thank you for contributing to the project! 💜 Please make sure to: - Chat with us first if this is a big change - Open a new issue (or comment on an existing one) - We want to make sure you don't spend time implementing something we might have to say No to - Add unit tests - Mention any relevant issues in the PR description (e.g. "Fixes #123") Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate. --> ## Which problem is this PR solving? Before this PR, decisions made for traces were not stored in redis. It also wasn't part of the state machine when transition a trace from awaiting for decision to either decision keep or drop. ## Short description of the changes - Introduced DecisionDrop and DecisionKeep into the state machine - check against in-memory cache before fetching from Redis for trace status - run smartwrapper tests against redis in the ci - removed some unnecessary code
kentquirk
pushed a commit
that referenced
this pull request
Apr 4, 2024
<!-- Thank you for contributing to the project! 💜 Please make sure to: - Chat with us first if this is a big change - Open a new issue (or comment on an existing one) - We want to make sure you don't spend time implementing something we might have to say No to - Add unit tests - Mention any relevant issues in the PR description (e.g. "Fixes #123") Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate. --> ## Which problem is this PR solving? Before this PR, decisions made for traces were not stored in redis. It also wasn't part of the state machine when transition a trace from awaiting for decision to either decision keep or drop. ## Short description of the changes - Introduced DecisionDrop and DecisionKeep into the state machine - check against in-memory cache before fetching from Redis for trace status - run smartwrapper tests against redis in the ci - removed some unnecessary code
VinozzZ
added a commit
that referenced
this pull request
May 6, 2024
<!-- Thank you for contributing to the project! 💜 Please make sure to: - Chat with us first if this is a big change - Open a new issue (or comment on an existing one) - We want to make sure you don't spend time implementing something we might have to say No to - Add unit tests - Mention any relevant issues in the PR description (e.g. "Fixes #123") Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate. --> ## Which problem is this PR solving? Before this PR, decisions made for traces were not stored in redis. It also wasn't part of the state machine when transition a trace from awaiting for decision to either decision keep or drop. ## Short description of the changes - Introduced DecisionDrop and DecisionKeep into the state machine - check against in-memory cache before fetching from Redis for trace status - run smartwrapper tests against redis in the ci - removed some unnecessary code
VinozzZ
added a commit
that referenced
this pull request
May 6, 2024
<!-- Thank you for contributing to the project! 💜 Please make sure to: - Chat with us first if this is a big change - Open a new issue (or comment on an existing one) - We want to make sure you don't spend time implementing something we might have to say No to - Add unit tests - Mention any relevant issues in the PR description (e.g. "Fixes #123") Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate. --> ## Which problem is this PR solving? Before this PR, decisions made for traces were not stored in redis. It also wasn't part of the state machine when transition a trace from awaiting for decision to either decision keep or drop. ## Short description of the changes - Introduced DecisionDrop and DecisionKeep into the state machine - check against in-memory cache before fetching from Redis for trace status - run smartwrapper tests against redis in the ci - removed some unnecessary code
tdarwin
pushed a commit
to tdarwin/refinery
that referenced
this pull request
Jun 26, 2024
…bio#1020) <!-- Thank you for contributing to the project! 💜 Please make sure to: - Chat with us first if this is a big change - Open a new issue (or comment on an existing one) - We want to make sure you don't spend time implementing something we might have to say No to - Add unit tests - Mention any relevant issues in the PR description (e.g. "Fixes honeycombio#123") Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate. --> ## Which problem is this PR solving? Before this PR, decisions made for traces were not stored in redis. It also wasn't part of the state machine when transition a trace from awaiting for decision to either decision keep or drop. ## Short description of the changes - Introduced DecisionDrop and DecisionKeep into the state machine - check against in-memory cache before fetching from Redis for trace status - run smartwrapper tests against redis in the ci - removed some unnecessary code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Before this PR, decisions made for traces were not stored in redis. It also wasn't part of the state machine when transition a trace from awaiting for decision to either decision keep or drop.
Short description of the changes