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

Add experimental metrics to e2e runs #2435

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Add experimental metrics to e2e runs #2435

merged 1 commit into from
Jan 19, 2022

Conversation

awgreene
Copy link
Member

@awgreene awgreene commented Nov 8, 2021

Adds counter metric for each sync performed by controller. Example:

$ curl -s localhost:8080/metrics | grep controller
# HELP controller_reconcile_adoption Number of reconciliations by the adoption controller
# TYPE controller_reconcile_adoption counter
controller_reconcile_adoption 1
# HELP controller_reconcile_operator Number of reconciliations by the operator controller
# TYPE controller_reconcile_operator counter
controller_reconcile_operator 0
# HELP controller_reconcile_operator_condition Number of reconciliations by the operator condition controller
# TYPE controller_reconcile_operator_condition counter
controller_reconcile_operator_condition 16
# HELP controller_reconcile_operator_condition_generator Number of reconciliations by the operator condition generator controller
# TYPE controller_reconcile_operator_condition_generator counter
controller_reconcile_operator_condition_generator 8

@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 Nov 8, 2021
@openshift-ci
Copy link

openshift-ci bot commented Nov 8, 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
Copy link

openshift-ci bot commented Nov 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2021
@awgreene awgreene requested review from tylerslaton and removed request for dinhxuanvu and gallettilance November 8, 2021 20:20
@awgreene
Copy link
Member Author

awgreene commented Nov 8, 2021 •
edited
Loading

Wanted to get a basic implementation up quickly, will be working on the variable names and the structure of the code.

@awgreene awgreene force-pushed the sync-metrics branch 4 times, most recently from 10277d3 to eac4737 Compare November 9, 2021 21:29
@awgreene awgreene force-pushed the sync-metrics branch 3 times, most recently from e734678 to 41d0002 Compare November 16, 2021 16:29
timflannagan
timflannagan reviewed Nov 16, 2021
View reviewed changes
Copy link
Contributor

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

Just a quick review + played around with this locally and was able to extract the metrics during an e2e run from the olm-operator deployment, and I think the build tag implementation is the right approach.

I had a couple of quick thoughts and comments:

  • Do we need to update the tes/e2e/README.md for how to interact with these metrics?
  • Should we scope down the introduction of these metrics to only CI until we have a need for them outside of ephemeral environments?

Makefile Outdated
@@ -84,6 +85,7 @@ build-coverage: clean $(CMDS)

build-linux: build_cmd=build
build-linux: arch_flags=GOOS=linux GOARCH=386
build-linux: BUILD_TAGS="json1 experimental_metrics"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea on why we need to update the build-linux target as well? At a glance locally, it doesn't look like this is used anywhere to drive CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the e2e test relies on the e2e-kind GitHub Workflow, which calls therun-local target here, which calls the build-linux target here.

This change might be better placed in the run-local target, thoughts?

pkg/metrics/experimental.go Outdated Show resolved Hide resolved
@awgreene awgreene marked this pull request as ready for review November 18, 2021 17:48
@awgreene awgreene changed the title WIP: add reconcile metrics Add experimental metrics to e2e runs Nov 18, 2021
@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 Nov 18, 2021
@awgreene awgreene force-pushed the sync-metrics branch 2 times, most recently from 6f20711 to 9220e09 Compare November 18, 2021 18:05
@awgreene
Copy link
Member Author

/retest

1 similar comment
@awgreene
Copy link
Member Author

/retest

@timflannagan
Copy link
Contributor

@awgreene It looks like the the DCO check still needs to be addressed.

@awgreene
Copy link
Member Author

/retest

@awgreene awgreene force-pushed the sync-metrics branch 2 times, most recently from eb5e80c to 00c9430 Compare December 22, 2021 14:18
@awgreene awgreene force-pushed the sync-metrics branch 4 times, most recently from 1f647b7 to a8bfed0 Compare January 18, 2022 20:27
@awgreene
Introduce Experimental Metrics
7650b0a 
This commit introduces a series of metrics that are only recorded when
OLM is built with the experimental_metrics build tag.

The metrics introduced in this commit includes namespace/name
information of the request being reconciled by a series of controllers.
These experimental metrics are enabled by default in the e2e-local
target defined in the project's Makefile.

Signed-off-by: Alexander Greene <greene.al1991@gmail.com>
@perdasilva
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2022
@openshift-merge-robot openshift-merge-robot merged commit a07e3cc into operator-framework:master Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timflannagan timflannagan timflannagan left review comments

@njhale njhale Awaiting requested review from njhale

@tylerslaton tylerslaton Awaiting requested review from tylerslaton

Assignees

@perdasilva perdasilva

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.
4 participants
@awgreene @timflannagan @perdasilva @openshift-merge-robot