Support LATENCY LATEST, LATEST HISTORY command parsing

[dmitri-lerko]

Includes tests similar to how SlowLog was tested.

[dmitri-lerko]

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]

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]

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]

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]

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.

[dmitri-lerko]

@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]

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]

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

Thanks Dmitri

[stevenh]

Thanks for PR @dmitri-lerko much appreciated.

[dmitri-lerko]

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.