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.

Sign up for GitHub

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

Jump to bottom

ApiRequestCount conditional gathering #492

Merged
merged 5 commits into from
Sep 13, 2021
Merged

ApiRequestCount conditional gathering #492

merged 5 commits into from
Sep 13, 2021

Conversation

tremes
Copy link
Contributor

@tremes tremes commented Sep 7, 2021 •
edited
Loading

This is adding a new conditional gatherer. If the APIRemovedInNextEUSReleaseInUse alert is firing then it looks at its labels, get the resource, version and group from them and finds the corresponding APIRequestCounts "record" for this resource.

Categories

  • Bugfix
  • Enhancement
  • Backporting
  • Others (CI, Infrastructure, Documentation)

Sample Archive

  • docs/insights-archive-sample/conditional/api_request_counts.json

Documentation

  • docs/gathered-data.md

Unit Tests

  • pkg/gatherers/conditional/gather_api_request_count_test.go

Privacy

Yes. There are no sensitive data in the newly collected information.

Changelog

Breaking Changes

No

References

https://issues.redhat.com/browse/CCXDEV-5581
https://bugzilla.redhat.com/show_bug.cgi?id=???
https://access.redhat.com/solutions/???

@openshift-ci
Copy link

openshift-ci bot commented Sep 7, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2021
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2021
@tremes tremes changed the title WIP New conditional gatherer - basic structure ApiRequestCount conditional gathering Sep 8, 2021
@tremes tremes force-pushed the new_cond_gath branch 2 times, most recently from de44227 to 7deea7d Compare September 8, 2021 10:38
Serhii1011010
Serhii1011010 reviewed Sep 8, 2021
View reviewed changes
pkg/gatherers/conditional/gather_api_request_count.go Outdated
}

func (g *Gatherer) gatherAPIRequestCounts(ctx context.Context, alertName string) ([]record.Record, []error) {
var resources []string
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to make it a set to improve performance of the for loop lil bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean the https://github.com/openshift/insights-operator/pull/492/files#diff-768929cfc4894c47306f40d5abfcac538dd69ef7d32f7b0a347dc31ce6919660R79 right? Yeah I think I can use a set. Thanks

Serhii1011010
Serhii1011010 reviewed Sep 8, 2021
View reviewed changes
pkg/utils/slices.go Outdated Show resolved Hide resolved
@tremes tremes marked this pull request as ready for review September 9, 2021 06:21
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 9, 2021
@Serhii1011010
Copy link
Contributor

looks great

@rluders
Copy link
Contributor

rluders commented Sep 13, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

natiiix
natiiix approved these changes Sep 13, 2021
View reviewed changes
Copy link
Contributor

@natiiix natiiix left a comment

Choose a reason for hiding this comment

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

Tested locally according to the steps provided using the Service Mesh operator, which resulted in conditional/alerts/APIRemovedInNextEUSReleaseInUse/api_request_counts.json file being successfully generated after an hour.
/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Sep 13, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natiiix, rluders, tremes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [natiiix,rluders,tremes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit aaf16b0 into openshift:master Sep 13, 2021
tremes added a commit to tremes/insights-operator that referenced this pull request Sep 13, 2021
@tremes
ApiRequestCount conditional gathering (openshift#492)
8b5187b 
* ApiRequestCounts conditional gathering

* Test and minor updates according to the review

* Update name

* Fix also the schema

* Change Alert type to AlertLabels map
@openshift-ci openshift-ci bot mentioned this pull request Sep 13, 2021
Merged
4 tasks
openshift-merge-robot pushed a commit that referenced this pull request Oct 27, 2021
@tremes
[release-4.9] Bug 2003893: ApiRequestCount conditional gathering (#492)…
8e4cb34 
… (#496)

* ApiRequestCount conditional gathering (#492)

* ApiRequestCounts conditional gathering

* Test and minor updates according to the review

* Update name

* Fix also the schema

* Change Alert type to AlertLabels map

* Fix sample archive path
@tremes tremes deleted the new_cond_gath branch May 6, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Serhii1011010 Serhii1011010 Serhii1011010 left review comments

@natiiix natiiix natiiix approved these changes

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@rluders rluders

@natiiix natiiix

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tremes @Serhii1011010 @rluders @openshift-bot @natiiix @openshift-merge-robot