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

fix(alerts): update to display all alerts #101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsundquist
Copy link

@jsundquist jsundquist commented Sep 15, 2022

Previously alerts would only display the first 50 results. This change updates it so that alerts and incidents acts the same in fetching all available alerts stored within Ops Genie.

I plan on updating both the alert table and incidents table as well to allow users to filter the results a little better as well. This is the initial step needed. Essentially right now the load time is incredible to load up both all incidents and alerts. One idea I have for the two tables is to initially only load those incidents or alerts that are open, after that allow users to see either all, open, closed, or resolved for incidents and open, acknowledged or closed for alerts.

Previously alerts would only dispaly the first 50 results.  This
change updates it so that alerts and incidents acts the same in
fetching all available alerts stored within Ops Genie.
Copy link
Owner

@K-Phoen K-Phoen 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 opening this PR!

I absolutely agree with you, both the incidents and alerts lists could use some love. Being able to explore all the data with decent filtering capabilities would indeed be a good start.

I also appreciate your effort to start with a smaller PR, taking one step in that direction.

That being said, this PR changes the behavior of the alerts table and the EntityOpsgenieAlertsCard component in a way that would make both unusable, so I'm reluctant to merge it as-is.

Instead of loading as many alerts as we can in memory and then displaying them, what if we made the table a bit more dynamic?
With the "remote data" feature of material table, we should be able to load small chunks of data at a time, while still being able to retain filtering/sorting capabilities. WDYT?

let response = await this.fetch<AlertsResponse>(`/v2/alerts?limit=${limit}&sort=${sort}&order=${order}${query}`);
let alerts = response.data;

while (response.paging.next) {
Copy link
Owner

Choose a reason for hiding this comment

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

This will change the behavior of the EntityOpsgenieAlertsCard component.
It currently displays only a maximum of 3 alerts for a given entity, but this change will make it display every alerts related to that entity.

This is because AFAIK, the limit parameter accepted by Opsgenie applies to a result page, not the overall search.

@jsundquist
Copy link
Author

jsundquist commented Oct 24, 2022

Thanks for opening this PR!

Of course, sorry for taking so long to reply back to your comments.

I absolutely agree with you, both the incidents and alerts lists could use some love. Being able to explore all the data with decent filtering capabilities would indeed be a good start.

👍

I also appreciate your effort to start with a smaller PR, taking one step in that direction.

That being said, this PR changes the behavior of the alerts table and the EntityOpsgenieAlertsCard component in a way that would make both unusable, so I'm reluctant to merge it as-is.

Completely agree with this statement. I was a little hesitant about putting it forth as I knew it would be pushing it a bit further than what the component currently can handle. It also would pull in more data that what most browsers could handle depending on the number of alerts a system has triggered.

Instead of loading as many alerts as we can in memory and then displaying them, what if we made the table a bit more dynamic? With the "remote data" feature of material table, we should be able to load small chunks of data at a time, while still being able to retain filtering/sorting capabilities. WDYT?

Using the remote data table looks like it could be a great option to remove the breaking nature that this PR puts forth. I'll take a look at material table to implement the remote data fetch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants