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

Bug: don't report build failure if network has problem #61

Closed
vatioz opened this Issue Dec 15, 2017 · 23 comments

Comments

3 participants
@vatioz
Copy link

vatioz commented Dec 15, 2017

We use anyStatus Desktop (v 1.4.101) for Jenkins builds, but I think this may be applied more generally.

Our use case:
We use VPN for connecting to our build server. Unfortunately VPN client is not able to maintain the connection in the case of any network disturbance and therefore we lose connection to build server very often (as we are changing network interfaces when (un)plugging to docking stations).

The problem:
In such case AnyStatus will report failure for all build jobs defined.
Not only this is very annoying as notifications will start blinking one after the other for good amount of time, but it is plain wrong. Network unavailability really does not equal build failure.

Solution:
My suggestion would be to use another state for network unavailability. Probably some gray icon or whatever. Also to not notify separately for each build when network is lost or revived. Rather don't show any notification in either case.

Note that I want to be notified about real build failures and stuff, so disabling notifications would not be an option here.

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 15, 2017

You're absolutely right, it is annoying.
I've been searching for a proper solution to detect such cases.
There's a difference between network unavailability and internet unavailability.
Can you please describe what are the conditions in your case or how it can be detected?

@AlonAm AlonAm added the bug label Dec 17, 2017

@vatioz

This comment has been minimized.

Copy link

vatioz commented Dec 18, 2017

Hey,
what I would assume is that unless you get proper response from the server (jenkins) that would tell you whether the build succeeded or failed, you cannot tell what is the current state, hence unknown state.

So if the request comes back, you can set success or failure.
If the request fails (i.e, 404, 500 HTTP responses or it times out, ...) you can only set unknown status.

Am I correct? What do you think?

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 19, 2017

That's an interesting approach.

So you suggest,

  1. When a network (404, 500, etc.) error occurs, set the status to Unknown instead of Error.
  2. Bypass notifications when the status changes to Unkown.

Note, when a build fails, the status is Failed, not Error.

I think it can work...

@vatioz

This comment has been minimized.

Copy link

vatioz commented Dec 19, 2017

Oh, so there is already a difference? So there is FAIL state and there is ERROR state right now. Is that correct?
So maybe only some option "Notify error states" would suffice?
Because what would be the difference between UNKNOWN and ERROR then? Are there usages for both?

Also, not only bypass notifications in the OK->UNKNOWN direction but in the opposite as well - UNKNOWN->OK.
Although, this opposite direction may get a bit tricky, because there may be some failed builds when connection is revived. Do we want to be notified about them? Ultimate solution would be to have last states saved and then compare them with revived ones - and notify only about the changed ones.

So let's say I have 3 builds. Connection is good and 2 are passing, 1 is failing.
Connection goes down.
No notification about builds. (All builds are in either ERROR or UNKNOWN state.)
Connection goes up. Builds don't change, 2 are passing, 1 is failing, (the same ones).
No notification about builds.
Connection goes down.
No notification
Connection goes up after some time. Builds change - all failing now.
I would get 2 notifications - I am interested only in the delta from the last known status ;)

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 19, 2017

@AlonAm

This comment has been minimized.

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 19, 2017

there's also a boolean property here that indicates whether a notification is needed

return ShowNotifications &&
           PreviousState != null &&
           PreviousState != State &&
           PreviousState != State.None;
@vatioz

This comment has been minimized.

Copy link

vatioz commented Dec 20, 2017

Well, it seems to me that what I am talking about matches current ERROR state. Skipping notification in transition from ERROR to different state is good. So the only missing thing would be

option to skip notifications in transition from something to ERROR state

That is enough to keep me happy (I think :) ).

I would then suggest extending IsNotificationRequired property in this manner:

return ShowNotifications &&
           (State != State.Error || ShowErrorNotifications)
           PreviousState != null &&
           PreviousState != State &&
           PreviousState != State.None;
@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 20, 2017

great!
do you think it should be disabled by default?

@vatioz

This comment has been minimized.

Copy link

vatioz commented Dec 20, 2017

If this applies generally and not specifically to builds, I would leave the option disabled by default (keeping current behavior; showing notifications on error).

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 20, 2017

cool, ill add the new property (show error notifications) to all plugins, with True as default to keep current behavior, and override it for builds and releases (false) to apply the new behavior

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 20, 2017

for the record, there are other solutions such as monitoring network availability or monitoring a queue of notifications but for now lets try your solution and see how it goes :-)

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 20, 2017

Further down the road, the user will be able to configure how to respond to status changes in different ways, like showing a notification or invoking some action

@AlonAm AlonAm changed the title Don't report build failure if network has problem Bug: don't report build failure if network has problem Dec 20, 2017

@AlonAm AlonAm added this to the 1.5 milestone Dec 20, 2017

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 20, 2017

the fix has been released in anystatus desktop 1.5.7
let's give it a try and see how it goes...

@vatioz

This comment has been minimized.

Copy link

vatioz commented Dec 21, 2017

Nice! That was fast! Thanks. I can confirm that I no longer get notifications for transitions to ERROR state.

But, I get them when I get back on vpn (transition from ERROR to some other state). That I wasn't expecting.
image

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 21, 2017

Hey there @SamuelEnglard,

In TeamCity Plugin, you've modified the code to bypass notifications when the network connection is restored (thank you for that!) and we would like to apply that behaviour to all other builds.

Did it reduce the number annoying notifications when the network connection is restored?
Why did you choose to handle it only when State.Failed (build failed)?
Should'nt we completely bypass notifications when state changes from Error to something else?
thank you! :-)

public override Notification CreateNotification()
        {
            if (State == State.Failed)
            {
                //todo: move "if" to API
                if (PreviousState == State.Error) return Notification.Empty; //bypass when network connection is restored.
                return new Notification($"{Name} failed. {StateText}", NotificationIcon.Error);
            }

            return base.CreateNotification();
}
@SamuelEnglard

This comment has been minimized.

Copy link

SamuelEnglard commented Dec 21, 2017

@AlonAm While I'd love to take credit for that, you did that one 😉

AnyStatus/Plugins@3fa2f87

My change was about showing status text on failure, so that's all I checked for.

AnyStatus/Plugins@4e9294b

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 21, 2017

@SamuelEnglard apologies, it must have been 2am ;-)

@SamuelEnglard

This comment has been minimized.

Copy link

SamuelEnglard commented Dec 21, 2017

@AlonAm it's ok, I write some of my best code at 2am!

@SamuelEnglard

This comment has been minimized.

Copy link

SamuelEnglard commented Dec 21, 2017

Though as I read through this thread I feel like this should be something handled by Build so that all get it "for free". There's no reason each sub class should be implementing this

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 21, 2017

You're right, i have added an option to show/hide error notifications for all widgets and set the default for builds to false

@AlonAm

This comment has been minimized.

Copy link
Member

AlonAm commented Dec 22, 2017

so I have released a new version (desktop 1.5.12) with the new behavior, including skipping notifications when errors recover.
lets give it a try and see how it goes...
thanks guys for the feedback <3

@vatioz

This comment has been minimized.

Copy link

vatioz commented Dec 22, 2017

I installed new version and I can confirm it is working as expected. Great!

@AlonAm AlonAm closed this Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment