Azure SD Failure metric and 404 Handling

[David-N-Perkins]

For issue 10455. In Azure service discovery:

• Added a metric to count failures
• When looking up Nics, for 404 responses log and continue instead of returning an error

No existing unit tests, so needs manual testing.

[David-N-Perkins]

@roidelapluie Please review when you have the time.

[peterbourgon]

Functions/methods should always return errors as the abstract error interface.

Suggested change

	func (client *azureClient) getNetworkInterfaceByID(ctx context.Context, networkInterfaceID string) (*network.Interface, *autorest.DetailedError) {
	func (client *azureClient) getNetworkInterfaceByID(ctx context.Context, networkInterfaceID string) (*network.Interface, error) {

[David-N-Perkins]

I don't see any advantages to doing this and several drawbacks.

1. The calling code has to use errors.As() to try and get a DetailedError, which would require another failure check.
2. There's also no indication from the method's signatures that a DetailedError would get returned. That information gets hidden to the caller. This makes code maintenance and future development harder.

I see how this would make sense if several different errors could be returned. Am I missing something else?

[peterbourgon]

In Go, fallible functions return error and never concrete types. The complexities that arise from concrete error return types are, well, far too numerous to put into a GitHub PR comment 😉

The calling code has to use errors.As()

This is standard operating procedure.

There's also no indication from the method's signatures that a DetailedError would get returned.

It's expected that this kind of thing is captured in method documentation.

[David-N-Perkins]

Do you have any references that go into further details?

[roidelapluie]

Can we narrow down the error to ignore in getNetworkInterfaceByID? This particular error would ignorer 404 in 3 different calls.

[peterbourgon]

If callers need to access specific details from the error, then you can return an error type in the function...

    case code was 404:
        return nil, &DetailedError{...}

And use errors.As at the callsite...

var detailedErr *DetailedError
iface, err := c.getNetworkInterfaceByID(ctx, id)
switch {
case err == nil:
    // handle success case
case errors.As(err, &detailedErr):
    // handle DetailedError case
case err != nil:
    // handle default error case
}

Or...

if iface, err := c.getNetworkInterfaceByID(ctx, id); err == nil { 
    // handle success case
} else if detailedErr := &(DetailedError{}); errors.As(err, &detailedErr) {
    // handle DetailedError case
} else if err != nil {
    // handle default error case
}

ref

[David-N-Perkins]

Yes, I found similar patterns when researching custom errors after your first post. I asked, because I didn't see any advantage to creating custom errors over the current implementation of just returning a DetailedError.

[peterbourgon]

It’s a good idea for functions that return errors always to use the error type in their signature (as we did above) rather than a concrete type such as *MyError, to help guarantee the error is created correctly. As an example, os.Open returns an error even though, if not nil, it’s always of concrete type *os.PathError.

reference, reference, etc.

[David-N-Perkins]

Thank you for the references. They were very helpful.

[David-N-Perkins]

I've updated this method to return the error interface with different concrete types. The 404 check is performed here instead of by the caller.

[peterbourgon]

If you need to access elements of a specific error type, use errors.As.

[David-N-Perkins]

Fixed.

[gburton1]

We built and deployed the version in this PR. The results looked good. The happy path with no issues works fine, and we forced an error condition, and the failedCount metric is working as expected.

2022-03-28T22:22:02.970028236Z stderr F ts=2022-03-28T22:22:02.969Z caller=refresh.go:98 level=error component="discovery manager scrape" discovery=azure msg="Unable to refresh target groups" err="could not get virtual machines: could not list virtual machines: compute.VirtualMachinesClient#ListAll: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code=\"InvalidSubscriptionId\" Message=\"The provided subscription identifier '4d' is malformed or invalid.\""

[roidelapluie]

@peterbourgon do you want to give this a last review?

[peterbourgon]

LGTM

[roidelapluie]

Thanks! Errors should not start with an uppercase, that is my last nit. Thanks @peterbourgon for the extra review.

[roidelapluie]

Suggested change

	var errorNotFound = errors.New("Network interface does not exist")
	var errorNotFound = errors.New("network interface does not exist")

[David-N-Perkins]

Fixed.

[roidelapluie]

Thanks!!