3
\$\begingroup\$

I am parsing the responses from our Varnish load balancers in order to monitor the status of various nodes. One of the responses we get is text consisting of multiple blocks of data, one for each server in the load balancer.

This is one such block:

Backend web05 is Healthy
Current states  good: 10 threshold:  8 window: 10
Average responsetime of good probes: 0.010285
Oldest                                                    Newest
================================================================
4444444444444444444444444444444444444444444444444444444444444444 Good IPv4
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX Good Xmit
RRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRR Good Recv
HHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHH-HHH-HHHHHHH---HHHHHHHH Happy

That is repeated (with no specific delimiters) for every backend there is. Each line is a string in a List<string> of all lines in the response.

Each Node represents a Varnish server, to which I have added the ability to pull back the above health status message.

The parser accepts a List<Backend> that will be populated or updated with all the found backends, and a List<string> containing all the lines received from the server. Backend is a POCO

Here is are the two classes required to test. Following that is a small sample program that will test it. No external libraries are required and I have removed all the actual remote network calls and connection processes, as these are not pertinent to my text-parsing code. The test program contains a sample of the response from the production services.

Main Classes

namespace VarnishTest
{
        using System.Collections.Generic;
        using System.Linq;
        using System.Text.RegularExpressions;

        public class Backend
        {
            public string Name { get; set; }
            public string Status { get; set; }
            public int TotalCount { get; set; }
            public int OkCount { get; set; }
            public float AvgResponse { get; set; }
            public List<bool> GoodIPv4 { get; set; }
            public List<bool> GoodXmit { get; set; }
            public List<bool> GoodRecv { get; set; }
            public List<bool> ErrorRecv { get; set; }
            public List<bool> Happy { get; set; }

            public Backend()
            {
                this.GoodIPv4 = new List<bool>();
                this.GoodXmit = new List<bool>();
                this.GoodRecv = new List<bool>();
                this.ErrorRecv = new List<bool>();
                this.Happy = new List<bool>();
            }
        }

        public class Node
        {
            public string Name { get; set; }
            public List<Backend> Backends { get; set; }

            public static void ParseDebugListResponse(List<Backend> backends, List<string> response)
            {
                int n = 1; // Skip the first line, which is the response code
                while (n < response.Count)
                {
                    // Spin until the start of a Backend block
                    if (!response[n].StartsWith("Backend "))
                    {
                        n++;
                        continue;
                    }

                    var summary = response[n++];
                    var states = response[n++];
                    var averages = response[n++];

                    var goodIp4 = string.Empty;
                    var goodXmit = string.Empty;
                    var goodRecv = string.Empty;
                    var errorRecv = string.Empty;
                    var happy = string.Empty;

                    // Skip the header rows
                    if (response[n].StartsWith("Oldest"))
                    {
                        n = n + 2;
                    }

                    if (response[n].EndsWith(" Good IPv4"))
                    {
                        goodIp4 = response[n++];
                    }

                    if (response[n].EndsWith(" Good Xmit"))
                    {
                        goodXmit = response[n++];
                    }

                    if (response[n].EndsWith(" Error Recv"))
                    {
                        errorRecv = response[n++];
                    }

                    if (response[n].EndsWith(" Good Recv"))
                    {
                        goodRecv = response[n++];
                    }

                    if (response[n].EndsWith(" Happy"))
                    {
                        happy = response[n++];
                    }

                    // Backend {NAME} is {STATUS}
                    var pattern = new Regex(@"Backend (?<name>\S+) is (?<status>\S+)");
                    var match = pattern.Match(summary);
                    var name = match.Groups["name"].Value;
                    var status = match.Groups["status"].Value;

                    if (backends.All(b => b.Name != name))
                    {
                        backends.Add(new Backend() { Name = name });
                    }

                    var backend = backends.Find(b => b.Name == name);

                    backend.Status = status;

                    // Current states  good: {GOOD} threshold:  {THRESHOLD} window: {WINDOW}
                    pattern = new Regex(@"Current states\s+good:\s+(?<good>\d+)\s+threshold:\s+(?<threshold>\d+)\s+window:\s+(?<window>\d+)");
                    match = pattern.Match(states);
                    backend.OkCount = int.Parse(match.Groups["good"].Value);
                    backend.TotalCount = int.Parse(match.Groups["window"].Value);

                    // Average responsetime of good probes: {AVERAGE}
                    pattern = new Regex(@"Average responsetime of good probes: (?<average>[\d\.]+)");
                    match = pattern.Match(averages);
                    backend.AvgResponse = float.Parse(match.Groups["average"].Value);

                    backend.GoodIPv4 = ParseHealthFlagsResponse(goodIp4, '4');
                    backend.GoodXmit = ParseHealthFlagsResponse(goodXmit, 'X');
                    backend.GoodRecv = ParseHealthFlagsResponse(goodRecv, 'R');
                    backend.ErrorRecv = ParseHealthFlagsResponse(errorRecv, 'X');
                    backend.Happy = ParseHealthFlagsResponse(happy, 'H');
                }
            }

            private static List<bool> ParseHealthFlagsResponse(string input, char trueChar)
            {
                var retval = new List<bool>();

                // A valid health line has a space to separate the values from the name
                if (!input.Contains((" "))) return retval;

                foreach (var x in input.Substring(0, input.IndexOf(" ")))
                {
                    retval.Add(x == trueChar);
                }
                return retval;
            }

        }
    }

Test Program

The test program is not part of the code review - I've just lifted some relevant parts from my unit tests to hopefully add some clarity, and to make it easy to reproduce.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;

namespace VarnishTest
{
    class Program
    {
        private static readonly List<string> StandardResponse = new List<string>()
        {
            "200 OK",
            "Backend web01 is Healthy",
            "Current states  good: 11 threshold:  1 window: 11",
            "Average responsetime of good probes: 0.100000",
            "Oldest                                                    Newest",
            "================================================================",
            "4444444444444444444444444444444444444444444444444444444444444444 Good IPv4",
            "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX-XXXXXXXXXXXXXXXXXXXXXX Good Xmit",
            "RRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRR Good Recv",
            "HHHHHHHHHHHHHHHHHHHHHHH-HHHHHHHHHHHHHH-HHHHHHHHHHHHHHHHHHHHHHHHH Happy",
            "Backend web02 is Healthy",
            "Current states  good: 12 threshold:  2 window: 12",
            "Average responsetime of good probes: 0.200000",
            "Oldest                                                    Newest",
            "================================================================",
            "4444444444444444444444444444444444444444444444444444444444444444 Good IPv4",
            "XXXXXXXXXXXXXXXXXXXXXXXXXXX-XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX Good Xmit",
            "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX Error Recv",
            "RRRRRRRRRRRRRRRRRRRRR-RRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRR Good Recv",
            "HHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHH-HHHHHHHHHHHHHHHHHHHHHHHHHHHHHH Happy",
            "Backend web03 is Healthy",
            "Current states  good: 13 threshold:  3 window: 13",
            "Average responsetime of good probes: 0.300000",
            "Oldest                                                    Newest",
            "================================================================",
            "4444444444444444444444444-44444444444444444444444444444444444444 Good IPv4",
            "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX-XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX Good Xmit",
            "RRRRRRRRRRRRRRRRRRRRRRRRR-RRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRR Good Recv",
            "HHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHHH--HHHHHHHHHHHHHHHHHHHHHH Happy"
        };

        static void Main(string[] args)
        {
            var backends = new List<Backend>();

            Node.ParseDebugListResponse(backends, StandardResponse);
            Debug.Assert(3 == backends.Count());
            Debug.Assert(1 == backends.Count(b => b.Name == "web01"));
            Debug.Assert(1 == backends.Count(b => b.Name == "web02"));
            Debug.Assert(1 == backends.Count(b => b.Name == "web03"));

            var n2 = backends.Find(b => b.Name == "web02");
            Debug.Assert(Math.Abs((double)0.200000 - (double)n2.AvgResponse) < 0.01);
            Debug.Assert(64 == n2.GoodIPv4.Count(h => h));
            Debug.Assert(63 == n2.GoodXmit.Count(h => h));
            Debug.Assert(64 == n2.ErrorRecv.Count(h => h));
            Debug.Assert(63 == n2.GoodRecv.Count(h => h));
            Debug.Assert(63 == n2.Happy.Count(h => h));

            var n3 = backends.Find(b => b.Name == "web03");
            Debug.Assert(Math.Abs((double)0.300000 - (double)n3.AvgResponse) < 0.01);
            Debug.Assert(63 == n3.GoodIPv4.Count(h => h));
            Debug.Assert(63 == n3.GoodXmit.Count(h => h));
            Debug.Assert(0 == n3.ErrorRecv.Count(h => h));
            Debug.Assert(63 == n3.GoodRecv.Count(h => h));
            Debug.Assert(62 == n3.Happy.Count(h => h));
        }
    }
}

The Problem

It feels a bit crude. I'm basically looping until I find the start of a block, then consuming line-by-line until the last line I'm interested in for that block.

Note that the number of response types can vary, for example there could be a line for "Error Recv", but that is not returned if there weren't any.

\$\endgroup\$
4
\$\begingroup\$

Your methods are too big and are doing too much. ParseDebugListResponse is responsible for finding the backend boundaries, splitting the response into backend objects, determining what each line is, and parsing each line. If you don't know about the Single-Responsibility Principle, you should check it out.

A solution needs to do a few things. Let's look first at how to split all of the input lines into blocks of Backend information:

public class MyNodeCollection : IEnumerable<Backend>
{
    private readonly List<Backend> _backends;

    public MyNodeCollection(List<string> list)
    {
        this._backends = new List<Backend>(this.FindBoundaries(list)
            .Select(s => new Backend(
                list
                .Skip(s.Item1)
                .Take(s.Item2 - s.Item1)
                .ToList())));
    }

    private IEnumerable<Tuple<int, int>> FindBoundaries(List<string> list)
    {
        return FindStarts(list)
            .Zip(FindEnds(list), (start, end) => Tuple.Create(start, end));
    }

    private IEnumerable<int> FindStarts(List<string> list)
    {
        return list
            .Select((s, i) => new { s, i })
            .Where(w => w.s.StartsWith("Backend "))
            .Select(ss => ss.i);
    }

    private IEnumerable<int> FindEnds(List<string> list)
    {
        return this.FindStarts(list)
            .Skip(1)
            .Select(s => s)
            .Concat(new List<int> { list.Count + 1 });
    }

    public IEnumerator<Backend> GetEnumerator()
    {
        return this._backends.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return this._backends.GetEnumerator();
    }
}

I've split the functionality into single-line methods making liberal use of Linq extension methods.

  • In FindStarts, all I'm doing is returning the indexes of lines that start with "Backend ". That is the first line of Backend data.
  • FindEnds is slightly more complicated in that I want to return the indexes of the last lines of Backend data. That's either the line before the next "Backed " line, or the last line in the list of strings.
  • In FindBoundaries, I need to combine the start indexes and the end indexes into tuples of indexes. I can do this using the little-used Zip method.
  • Finally, in the constructor I take the tuples of boundary indexes and use them to split the given list into chunks of Backend data.
  • Note that I have MyNodeCollection implement IEnumerable<Backend> so that your testing code will work without modification.

I've changed your Backend class to something that more closely follows the Single-Responsibility Principle. Most of the methods are only one line long. They all have meaningful names.

public class Backend
{
    public string Name { get; private set; }
    public string Status { get; private set; }
    public int TotalCount { get; private set; }
    public int OkCount { get; private set; }
    public float AvgResponse { get; private set; }
    public List<bool> GoodIPv4 { get; private set; }
    public List<bool> GoodXmit { get; private set; }
    public List<bool> GoodRecv { get; private set; }
    public List<bool> ErrorRecv { get; private set; }
    public List<bool> Happy { get; private set; }

    public Backend(List<string> list)
    {
        this.GoodIPv4 = new List<bool>();
        this.GoodXmit = new List<bool>();
        this.GoodRecv = new List<bool>();
        this.ErrorRecv = new List<bool>();
        this.Happy = new List<bool>();

        foreach (var line in list)
        {
            this.ParseLine(line);
        }
    }

    private void ParseLine(string line)
    {
        if (this.IsAverageResponseTime(line))
            this.AvgResponse = this.ParseAverageResponseTime(line);

        else if (this.IsErrorRecv(line))
            this.ErrorRecv.AddRange(this.ParseGoodErrorHappy(line, 'X'));

        else if (this.IsGoodIPv4Count(line))
            this.GoodIPv4.AddRange(this.ParseGoodErrorHappy(line, '4'));

        else if (this.IsGoodRecv(line))
            this.GoodRecv.AddRange(this.ParseGoodErrorHappy(line, 'R'));

        else if (this.IsGoodXmit(line))
            this.GoodXmit.AddRange(this.ParseGoodErrorHappy(line, 'X'));

        else if (this.IsHappy(line))
            this.Happy.AddRange(this.ParseGoodErrorHappy(line, 'H'));

        else if (this.IsName(line))
            this.Name = this.ParseName(line);
    }

    private string ParseName(string arg)
    {
        return Regex.Replace(arg, @"Backend (\S*) is .*", "$1");
    }

    private bool IsName(string arg)
    {
        return arg.StartsWith("Backend ");
    }

    private float ParseAverageResponseTime(string arg)
    {
        string str = Regex.Replace(arg, 
            @"Average responsetime of good probes: (.*)", "$1");
        return float.Parse(str);
    }

    private bool IsAverageResponseTime(string arg)
    {
        return arg.StartsWith("Average responsetime of good probes: ");
    }

    private IEnumerable<bool> ParseGoodErrorHappy(string arg, char ch)
    {
        return arg.Where((w, i) => i < 64).Select(s => s == ch);
    }

    private bool IsGoodIPv4Count(string arg)
    {
        return arg.EndsWith(" Good IPv4");
    }

    private bool IsGoodXmit(string arg)
    {
        return arg.EndsWith(" Good Xmit");
    }

    private bool IsGoodRecv(string arg)
    {
        return arg.EndsWith(" Good Recv");
    }

    private bool IsErrorRecv(string arg)
    {
        return arg.EndsWith(" Error Recv");
    }

    private bool IsHappy(string arg)
    {
        return arg.EndsWith(" Happy");
    }
}
  • The constructor loops through each line and sends it to ParseLine for parsing.
  • ParseLine does most of the work. It uses the rest of the methods to determine what kind of line it's looking at and then parses it.
  • The Is... and the remaining Parse... methods are straight-forward.
  • Take a look at how I use Regexes in ParseName and ParseAverageResponseTime. In your Regex calls, you use matches and groups to pull out the information you need. Instead of that, I use the Replace methods to pull out the required information. This turns what is three or four lines of code into a single line of code.
  • Your ParseHealthFlagsResponse method and my ParseGoodErrorHappy method do nearly the same thing. This illustrates the power of Linq extension methods. I suggest you become familiar with them.
\$\endgroup\$
  • \$\begingroup\$ Not sure I'm a big fan of lots of single-line methods that only get called once, it's like you're using method names as comments. For example, IsgoodIPv4Count could well be called DoesStringEndWithGoodIPv4 - that would even be frowned upon as a comment, let alone a separate method. I do like the way you find the single blocks in FindBoundaries and FindStarts/Ends - I'll have a look at that. \$\endgroup\$ – Cylindric Feb 3 '16 at 14:55
  • 1
    \$\begingroup\$ @Cylindric, The difference between IsgoodIPv4Count and DoesStringEndWithGoodIPv4 is the the first one tells you what the method does while the second tells you how it does what it does. As the caller, I don't care how it's determined that a given line is an IPv4 line, I just want to know whether or not it is. Down the road if the line changes from ending with "Good IPv4" to starting with "Good IPv4", I can change the implementation without changing the name. \$\endgroup\$ – user2023861 Feb 3 '16 at 16:15
  • 1
    \$\begingroup\$ @Cylindric, I'll make the case for using method names as comments. The problem with comments is that people tend to use them like code. They'll use comments to tell you what a piece of code does. Inevitably when the code changes, chances are slim that the coder will also change the comments. So the comments end up misleading or worse. I suggest you check out the book "Simple Code" by Robert C. Martin. He devotes a whole chapter to naming things and another to comments. \$\endgroup\$ – user2023861 Feb 3 '16 at 16:21
  • 1
    \$\begingroup\$ @Cylindric, here are some opinions to back me up programmers.stackexchange.com/questions/64449/… Short functions are readable, maintainable, and testable. \$\endgroup\$ – user2023861 Feb 3 '16 at 17:07
  • 1
    \$\begingroup\$ @Cylindric, I wouldn't do that either. One of the methods in my answer is 14 lines. Another is 9. I never suggested that I split them into one-line methods. A guideline is not a law. I have good reasons for how I've split my code into methods. As you become a more experienced developer you'll find that decisions that you thought were arbitrary like naming things, placing whitespace, and the length of code blocks actually deserve some thought. They can be leveraged to provide more useful structure to your code. \$\endgroup\$ – user2023861 Feb 3 '16 at 21:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.