Skip to content

Add new handler for metrics#3436

Merged
bharathi-tenneti merged 1 commit intooperator-framework:masterfrom
bharathi-tenneti:metrics_handler
Jul 16, 2020
Merged

Add new handler for metrics#3436
bharathi-tenneti merged 1 commit intooperator-framework:masterfrom
bharathi-tenneti:metrics_handler

Conversation

@bharathi-tenneti
Copy link
Copy Markdown
Contributor

@bharathi-tenneti bharathi-tenneti commented Jul 15, 2020

Description: Add new handler for metrics which can be registered with controller-runtime registry, and wraps existing EnqueueResuestForObject, for both Ansible and Helm controllers.

Motivation: This PR addresses current complexity around exposing primary resource metrics in SDK.

/kind feature

@bharathi-tenneti bharathi-tenneti changed the title Added new handler for metrics <WIP> Add new handler for metrics Jul 15, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020
@bharathi-tenneti bharathi-tenneti added language/ansible Issue is related to an Ansible operator project language/helm Issue is related to a Helm operator project kind/feature Categorizes issue or PR as related to a new feature. labels Jul 15, 2020
@bharathi-tenneti
Copy link
Copy Markdown
Contributor Author

bharathi-tenneti commented Jul 15, 2020

Metrics output for Ansible and Helm sample operators correspondingly.

# HELP primary_resource_created_at_seconds Timestamp at which a resource was created
# TYPE primary_resource_created_at_seconds gauge
primary_resource_created_at_seconds{group="cache.example.com",kind="Memcached",name="example-memcached",namespace="memcached",version="v1alpha1"} 1.594841815e+09
# HELP primary_resource_created_at_seconds Timestamp at which a resource was created
# TYPE primary_resource_created_at_seconds gauge
primary_resource_created_at_seconds{group="cache.example.com",kind="Memcached",name="example-memcached",namespace="helm-memcached",version="v1alpha1"} 1.594843122e+09

Copy link
Copy Markdown
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Looks great. Just a few minor nits.

Comment thread pkg/handler/instrumented_enqueue_object.go Outdated
Comment thread pkg/handler/instrumented_enqueue_object.go Outdated
Comment thread pkg/handler/internal/metrics/metrics.go
Comment thread pkg/handler/internal/metrics/metrics.go Outdated
@joelanford
Copy link
Copy Markdown
Member

We should probably document this metric somewhere in the helm and ansible docs as well.

@bharathi-tenneti bharathi-tenneti changed the title <WIP> Add new handler for metrics Add new handler for metrics Jul 15, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020
Comment thread pkg/handler/instrumented_enqueue_object.go Outdated
Comment thread pkg/handler/instrumented_enqueue_object.go
Comment thread pkg/handler/internal/metrics/metrics.go Outdated
Comment thread pkg/ansible/controller/controller.go Outdated
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Hi @bharathi-tenneti,

It shows great 🥇 Just a few nits.

Comment thread pkg/handler/instrumented_enqueue_object.go
Comment thread pkg/handler/internal/metrics/metrics.go
Comment thread pkg/ansible/controller/controller.go
@joelanford
Copy link
Copy Markdown
Member

On second thought, let's add the tests in a follow-up.

I've got a few other PRs I want to submit that depend on this merging.

@bharathi-tenneti
Copy link
Copy Markdown
Contributor Author

On second thought, let's add the tests in a follow-up.

I've got a few other PRs I want to submit that depend on this merging.

@joelanford @camilamacedo86
Okay, I will add a TODO doc, for the tests, and raise a story to keep track. Anything else we want to do in this PR?

@joelanford joelanford mentioned this pull request Jul 16, 2020
92 tasks
*Added changelog fragment
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm
Since we agreed in do in a follow up the tests and docs improvements.

Great work 👍

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
Copy link
Copy Markdown
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@bharathi-tenneti bharathi-tenneti merged commit 9f6ccd6 into operator-framework:master Jul 16, 2020
jmrodri added a commit to jmrodri/operator-lib that referenced this pull request Jul 24, 2020
Moving instrumented_enqueue_object from operator-sdk:
operator-framework/operator-sdk#3436
jmrodri added a commit to jmrodri/operator-lib that referenced this pull request Jul 24, 2020
Moving instrumented_enqueue_object from operator-sdk:
operator-framework/operator-sdk#3436
jmrodri added a commit to operator-framework/operator-lib that referenced this pull request Jul 24, 2020
Moving instrumented_enqueue_object from operator-sdk:
operator-framework/operator-sdk#3436
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature. language/ansible Issue is related to an Ansible operator project language/helm Issue is related to a Helm operator project lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants