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

Added state tracking and conditional event triggering for URL status … #212

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tarunchy
Copy link

…changes

Added state tracking and conditional event triggering for URL status changes

  • Added initialization for client_error to handle cases where an exception occurs.
  • Introduced url_states dictionary to track the last known status of each URL.
  • Modified the main loop to include state checking and trigger events only if the URL status has changed, preventing duplicate rule triggers when the URL status transitions from down to up.

These changes ensure more accurate state management and reduce unnecessary event triggers.

I work in large health insurance company who use this tool and we are facing issue. We found this could be the root cause we will also raise support ticket through format channel

tarunchy added 2 commits June 11, 2024 08:29
…changes

Added state tracking and conditional event triggering for URL status changes

- Added initialization for `client_error` to handle cases where an exception occurs.
- Introduced `url_states` dictionary to track the last known status of each URL.
- Modified the main loop to include state checking and trigger events only if the URL status has changed, preventing duplicate rule triggers when the URL status transitions from down to up.

These changes ensure more accurate state management and reduce unnecessary event triggers.
…L status has changed

Improve URL status polling by adding state tracking and conditional event triggering

- Initialize client_error to handle cases where an exception occurs
- Add url_states dictionary to track the last known status of each URL
- Modify event triggering to occur only if the URL status has changed
Copy link
Contributor

@Alex-Izquierdo Alex-Izquierdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @tarunchy

async with session.get(url, ssl=verify_ssl) as resp:
status = "up" if resp.status == OK else "down"
status_code = resp.status
except aiohttp.ClientError as exc:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to set the status to "down" is not correct, since the ClientError can be raised by a variety of reasons that doesn't necessarily mean the url is down, like for example networking issues in the client side. This would be something that the plugin is already doing wrong in the upper try/except block.

Instead, I suggest a new option with "false" as default named for example "send_error_events" and send the ClientError as event if its set.

thoughts? @bzwei

status = "down"
status_code = None
client_error = str(exc)
# Only trigger event if the status has changed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that logic should be handled by the plugin and it would change the existing behavior so it could break existing rulebooks.

Copy link

@ssbarnea ssbarnea added the enhancement New feature or request label Aug 16, 2024
@ssbarnea
Copy link
Member

@tarunchy Any updates? If is missing current release, it might be delayed notably.

@tarunchy
Copy link
Author

Apologies for the delay in response. We received a workaround from Red Hat support that appears to be working with our current code. The workaround involved increasing the delay in the url_check plugin to 360. It seems that this adjustment resolved the issue. The code I provided should work as well, but this alternative workaround also seems effective. Please let me know if any further action is required from my side, and I’m happy to assist.

@ssbarnea
Copy link
Member

When @Alex-Izquierdo is back from PTO, he will know what to do with PR. Thanks for the quick answer.

@Alex-Izquierdo
Copy link
Contributor

Hi, I commented some concerns about the current implementation that I would like to solve prior to merge it. If you are happy with the workaround and close this PR, that's up to you and would be fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants