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

Support LATENCY LATEST, LATEST HISTORY command parsing #614

Merged
merged 15 commits into from Jul 6, 2022

Conversation

dmitri-lerko
Copy link
Contributor

@dmitri-lerko dmitri-lerko commented Jun 4, 2022

Includes tests similar to how SlowLog was tested.

Copy link
Collaborator

@stevenh stevenh left a comment

Thanks for the PR, there's a few things I've noted below.

While I think this could be helpful I'm wondering how much use it would get and hence should we put these type of helpers in their own sub package to avoid polluting the main API?

Something like:
redis/latency which would provide calls like:

latency.Latest(result interface{}, err error) ([]LatestEvent, error)
latency.History(result interface{}, err error) ([]HistoryEvent, error)

Thoughts?

redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply_test.go Outdated Show resolved Hide resolved
redis/reply_test.go Outdated Show resolved Hide resolved
redis/reply_test.go Outdated Show resolved Hide resolved
redis/reply_test.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
@dmitri-lerko
Copy link
Contributor Author

dmitri-lerko commented Jun 7, 2022

Thank you @stevenh, a lot of valuable suggestions and learning here! I'll aim to go through it over next couple of days. I'll aim to apply the same changes to SlowLogs helper/tests which I've based my change on.

@stevenh
Copy link
Collaborator

stevenh commented Jun 7, 2022

Thank you @stevenh, a lot of valuable suggestions and learning here! I'll aim to go through it over next couple of days. I'll aim to apply the same changes to SlowLogs helper/tests which I've based my change on.

I would encourage you do the changes to SlowLogs in a different PR, to avoid confusing issues.

@dmitri-lerko
Copy link
Contributor Author

dmitri-lerko commented Jun 11, 2022

Thanks for the PR, there's a few things I've noted below.

While I think this could be helpful I'm wondering how much use it would get and hence should we put these type of helpers in their own sub package to avoid polluting the main API?

Something like: redis/latency which would provide calls like:

latency.Latest(result interface{}, err error) ([]LatestEvent, error)
latency.History(result interface{}, err error) ([]HistoryEvent, error)

Thoughts?

When developing a tool to logout slowlog entries I have failed to discover that there was a SlowLogs helper already implemented and spent some time implementing my own. I concerned that if this change introduce new pattern of latency. then it will further hinder discoverability of the APIs.

latency.Latest(result interface{}, err error) ([]LatestEvent, error)
latency.History(result interface{}, err error) ([]HistoryEvent, error)

@dmitri-lerko
Copy link
Contributor Author

dmitri-lerko commented Jun 11, 2022

I've applied suggested changes in all the instances that I could find. Tests look a lot more dense and readable.

Not sure what is more usable:

	resultStr, err := redis.Strings(c.Do("CONFIG", "GET", "latency-monitor-threshold"))
	require.NoError(t, err, "TestLatency failed during CONFIG GET latency-monitor-threshold.")

or

	resultStr, err := redis.Strings(c.Do("CONFIG", "GET", "latency-monitor-threshold"))
	require.NoError(t, err)

Framework does point at the problematic test, but lacking of particular details outside of the error message:

=== RUN   TestLatency
    reply_test.go:231:
        	Error Trace:	reply_test.go:231
        	Error:      	Received unexpected error:
        	            	server exited: 96422:M 11 Jun 2022 11:06:36.347 # Failed listening on port 16111 (TCP), aborting.
        	Test:       	TestLatency
        	Messages:   	TestLatency failed during dial.
--- FAIL: TestLatency (0.04s)

Additionally, I've renamed Event to EventType, but also happy to just change it to Name as per your original suggestion.

@stevenh
Copy link
Collaborator

stevenh commented Jun 14, 2022

I've applied suggested changes in all the instances that I could find. Tests look a lot more dense and readable.

Not sure what is more usable:

	resultStr, err := redis.Strings(c.Do("CONFIG", "GET", "latency-monitor-threshold"))
	require.NoError(t, err, "TestLatency failed during CONFIG GET latency-monitor-threshold.")

or

	resultStr, err := redis.Strings(c.Do("CONFIG", "GET", "latency-monitor-threshold"))
	require.NoError(t, err)

Framework does point at the problematic test, but lacking of particular details outside of the error message:

=== RUN   TestLatency
    reply_test.go:231:
        	Error Trace:	reply_test.go:231
        	Error:      	Received unexpected error:
        	            	server exited: 96422:M 11 Jun 2022 11:06:36.347 # Failed listening on port 16111 (TCP), aborting.
        	Test:       	TestLatency
        	Messages:   	TestLatency failed during dial.
--- FAIL: TestLatency (0.04s)

Additionally, I've renamed Event to EventType, but also happy to just change it to Name as per your original suggestion.

My take on is that in 99% of cases you get all the context needed from the test name TestLatency and the file:line at which it failed reply_test.go:231 so there's no need to add addition context.

If you added the message TestLatency failed during CONFIG GET latency-monitor-threshold and the function was renamed then the message is wrong, it's also redundant information as the test output already includes that, so I would never name the test in the message.

Adding CONFIG GET latency-monitor-threshold does inform the user what failed when the tests runs, however that is unlikely to help them fix the issue, that needs additional context of the surrounding code, which can already be found from file:line.

The way I look at it is use a message where it helps the developer understand why this check exists if it's not clear, which for almost all cases is already obvious particularly so for error check.

Another thing in that snippet is the variable naming resultStr. As a typed language it's not considered idiomatic to put type in the variable name unless you need to disambiguate with other variables. In this case it's also misleading as the result of redis.Strings() is actually a slice not a string. You should choose a name which adds enough context for the scope that is used. For example in this case I would just use res as it's only needed for one line below.

As a full example I would go for the following in this particualar case:

	res, err := redis.Strings(c.Do("CONFIG", "GET", "latency-monitor-threshold"))
	require.NoError(t, err)
	if len(res) == 0 {
		t.Skip("Latency commands not supported, added in redis 2.8.13")
	}

Hope that helps.

Copy link
Collaborator

@stevenh stevenh left a comment

Making some great progress, here's some more pointers.

Personally I would remove all the require messages as per my reply to your question.

If you compare with and without, you'll see the code is much less busy which improves readability, without removing any understanding for a developer looking at any potential failure based.

redis/reply.go Show resolved Hide resolved
redis/reply.go Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply_test.go Outdated Show resolved Hide resolved
redis/test_test.go Outdated Show resolved Hide resolved
redis/reply_test.go Outdated Show resolved Hide resolved
redis/reply_test.go Outdated Show resolved Hide resolved
redis/reply_test.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
@dmitri-lerko
Copy link
Contributor Author

dmitri-lerko commented Jun 18, 2022

@stevenh , thank you for the time invested into this. I've learned a lot already and made another iteration incorporating your feedback. Let me know if there is anything else I can improve upon.

Thanks

@stevenh
Copy link
Collaborator

stevenh commented Jun 20, 2022

Thanks @dmitri-lerko if you could mark all the comments you have addressed at resolved, that will make it easier to do another review :)

@dmitri-lerko
Copy link
Contributor Author

dmitri-lerko commented Jun 21, 2022

Hi @stevenh, I've resolved all but one comments. Please let me know what you think.

Thanks
Dmitri

Copy link
Collaborator

@stevenh stevenh left a comment

Just some error message and comments to clean up and should be good to go.

redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
redis/reply.go Outdated Show resolved Hide resolved
@stevenh stevenh merged commit d685447 into gomodule:master Jul 6, 2022
9 checks passed
@stevenh
Copy link
Collaborator

stevenh commented Jul 6, 2022

Thanks for PR @dmitri-lerko much appreciated.

@dmitri-lerko dmitri-lerko deleted the feature/support-redis-latency branch Jul 6, 2022
@dmitri-lerko
Copy link
Contributor Author

dmitri-lerko commented Jul 6, 2022

Hi @stevenh , thank you for your time on this PR. I'll aim to bring slowlog in light with the changes above to have better consistency.

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.

None yet

2 participants