PR for Micrometer Integration - #64 #486

laxmikantbpandhare · 2021-08-06T19:06:58Z

Working on Timer and other remaining part. @metacosm and @csviri - Could you please verify the code for Counter and its implemented methods.

jmrodri · 2021-08-06T20:59:11Z

@laxmikantbpandhare will this tie into the deleteResource method for decrementing the metric? https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java#L28-L30

How will we ensure the increment occurs for the metrics we plan to support? Or is that for another follow on PR?

jmrodri · 2021-08-06T21:13:43Z

I think that we should create a metrics package in the operator-framework-core/src/main/java/io/javaoperatorsdk/operator or in operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api to house the metrics stuff. And I think CounterDetails should be in implementation detail of what we're trying to do. The issue lists a set of metrics we would like to expose by default.

number of controller executions
number of events processed
controller execution durations
number of custom resource managed
retry related metrics

So I think we should have a metrics.ControllerExecution which is an instance of a CounterDetails. We can then increment it for the user whenever a reconcile occurs. Similar to how controller-runtime is doing it here: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/internal/controller/metrics/metrics.go#L30-L33

	ReconcileTotal = prometheus.NewCounterVec(prometheus.CounterOpts{
		Name: "controller_runtime_reconcile_total",
		Help: "Total number of reconciliations per controller",
	}, []string{"controller", "result"})

As you see in controller-runtime, there are a series of default structures that represent the default metrics they want to expose. I could see us having 5 classes representing the metrics we want.

Then in the deleteResource we would decrement the ControllerExecution.

adam-sandor · 2021-08-09T08:18:50Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/utils/CounterDetails.java

+
+public class CounterDetails {
+
+    int counter;


do these fields need to be package-private instead of private?

checking on this, we can make those field as a private.

I don't currently see what's the point of this class since it doesn't appear to be used but I would assume that it should be immutable: all fields should be private and final and there shouldn't be any setters (i.e. it shouldn't be possible to modify an instance, after it's been created)

The counter details class will be used when we call/use these utility methods. You can refer to sample memcached-quarkus-operator where I used this - https://github.com/laxmikantbpandhare/memcached-quarkus-operator/blob/main/src/main/java/com/example/MemcachedController.java

laxmikantbpandhare · 2021-08-09T20:50:50Z

@laxmikantbpandhare will this tie into the deleteResource method for decrementing the metric? https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java#L28-L30

How will we ensure the increment occurs for the metrics we plan to support? Or is that for another follow on PR?

@jmrodri - I am still looking into the code of java-operator-sdk for delete resource functionality. I will add those changes as part of this PR only.

laxmikantbpandhare · 2021-08-11T02:48:46Z

@laxmikantbpandhare will this tie into the deleteResource method for decrementing the metric? https://github.com/java-operator-sdk/java-operator-sdk/blob/master/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java#L28-L30

How will we ensure the increment occurs for the metrics we plan to support? Or is that for another follow on PR?

@jmrodri - We will not decrement any counter as operator-sdk does not decrement anything as such. After delete event, the reconciliation process happens thats why need to increment controller execution counter. The operator-sdk has two counters, succeeded and failed. If reconciliation process fails then failed counter increases. We will follow the same logic for our metrics part in java-operator-sdk.

jmrodri · 2021-08-11T04:34:12Z

@jmrodri - We will not decrement any counter as operator-sdk does not decrement anything as such. After delete event, the reconciliation process happens thats why need to increment controller execution counter. The operator-sdk has two counters, succeeded and failed. If reconciliation process fails then failed counter increases. We will follow the same logic for our metrics part in java-operator-sdk.

You are correct we only increment. Thanks.

jmrodri · 2021-08-11T04:35:14Z

@laxmikantbpandhare also, this code should live in a metrics package.

laxmikantbpandhare · 2021-08-11T04:39:05Z

@laxmikantbpandhare also, this code should live in a metrics package.

@jmrodri - Sure, I will create metrics package and add those changes in it.

jmrodri · 2021-08-11T04:57:32Z

Also remember that PRs should follow the conventional commit: https://github.com/java-operator-sdk/java-operator-sdk/blob/master/CONTRIBUTING.md#pull-request-process

laxmikantbpandhare · 2021-08-11T05:19:49Z

Also remember that PRs should follow the conventional commit: https://github.com/java-operator-sdk/java-operator-sdk/blob/master/CONTRIBUTING.md#pull-request-process

Modified commit as a conventional commit.

metacosm · 2021-08-16T16:19:13Z

I've converted this PR to draft since it seems quite far from being finished. I cannot really review it in its current state because I need to see:

which types of metrics will be provided
how they will integrate with the SDK code
which data will be recorded

laxmikantbpandhare · 2021-08-17T07:13:39Z

@metacosm - I showed demo of metrics during our weekly meeting. Attached screen print for your reference.

The current changes are just utility methods and I am planning to call/use those methods from reconciliation process. This will calculate the successful execution of reconciliation process. You can see in above image, I marked that data in red box.

metacosm · 2021-08-17T09:13:46Z

On a design note: I'd like to see something similar to what is seen in https://github.com/ebullient/monster-combat/blob/main/core/src/main/java/dev/ebullient/dnd/combat/client/CombatMetrics.java. All metrics-related code is confined to a single class so that it doesn't "infect" the code and we can tweak what gets recorded and how in a single spot. The SDK code would call method of this class with application-specific results (e.g. UpdateControl) as opposed to things related to metrics, though we could add metrics-related information to these objects if needed.
Also, we should only depend on micrometer-core, not micrometer-registry-prometheus so that we're not dependent on a given backend but users can choose whichever backend they want.

laxmikantbpandhare · 2021-08-17T15:26:59Z

On a design note: I'd like to see something similar to what is seen in https://github.com/ebullient/monster-combat/blob/main/core/src/main/java/dev/ebullient/dnd/combat/client/CombatMetrics.java. All metrics-related code is confined to a single class so that it doesn't "infect" the code and we can tweak what gets recorded and how in a single spot. The SDK code would call method of this class with application-specific results (e.g. UpdateControl) as opposed to things related to metrics, though we could add metrics-related information to these objects if needed.
Also, we should only depend on micrometer-core, not micrometer-registry-prometheus so that we're not dependent on a given backend but users can choose whichever backend they want.

Yes, agree on this. I will update the code accordingly.

laxmikantbpandhare · 2021-08-18T04:05:16Z

On a design note: I'd like to see something similar to what is seen in https://github.com/ebullient/monster-combat/blob/main/core/src/main/java/dev/ebullient/dnd/combat/client/CombatMetrics.java. All metrics-related code is confined to a single class so that it doesn't "infect" the code and we can tweak what gets recorded and how in a single spot. The SDK code would call method of this class with application-specific results (e.g. UpdateControl) as opposed to things related to metrics, though we could add metrics-related information to these objects if needed.
Also, we should only depend on micrometer-core, not micrometer-registry-prometheus so that we're not dependent on a given backend but users can choose whichever backend they want.

@metacosm - I completely removed CounterDetails class and added entire logic in only one class. Please review the PR. About the second suggestion, micrometer-core contains only SimpleMeterRegistry and it is the simplest form of the registry. As per me, MeterRegistry is the best option. Below links explains more about it.

Links -
https://micrometer.io/docs/installing
https://www.baeldung.com/micrometer#3-registry

metacosm · 2021-08-18T05:59:50Z

We're not going to provide a registry implementation in the SDK: this will be up to users to decide which one to use.
More precisely: the SDK will not be doing any actual metering, just provide the infrastructure for it to happen when a user selects a backend and activates it in their operator. MeterRegistry is the abstract class we should be coding against, not any implementation.

laxmikantbpandhare · 2021-08-18T06:20:55Z

We're not going to provide a registry implementation in the SDK: this will be up to users to decide which one to use.
More precisely: the SDK will not be doing any actual metering, just provide the infrastructure for it to happen when a user selects a backend and activates it in their operator. MeterRegistry is the abstract class we should be coding against, not any implementation.

Ah! sorry it was my confusion. I updated dependency as said by you in Quarkus Memcached Operator and was getting UnSatisfiedDepedency for micrometer-core. It was my mistake. I updated thepom.xml file. Please review and let me know your comments.

laxmikantbpandhare · 2021-08-19T20:38:06Z

@laxmikantbpandhare can you make your branch writable by maintainers, please? PRs should be opened in a way that maintainers can change stuff on it.

I already tick marked Allow edits by maintainers.

laxmikantbpandhare · 2021-08-19T20:39:01Z

I've created #497 to show how I think we should go about the integration… Lots of work still to be done but should provide a basis for further discussions.

@metacosm @adam-sandor @csviri - I created similar method for deleteResource(). Please review and let me know your comments.

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Metrics.java

...tor-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java

...framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java

metacosm · 2021-08-20T11:01:06Z

On a strictly process-oriented basis: please do not request reviews for each commit. Finish the PR as you think it should be done and then we will be able to review it. Also, please leave the PR in draft mode until it's ready for review and merging, which isn't the case as this stage.

laxmikantbpandhare · 2021-08-20T19:31:31Z

On a strictly process-oriented basis: please do not request reviews for each commit. Finish the PR as you think it should be done and then we will be able to review it. Also, please leave the PR in draft mode until it's ready for review and merging, which isn't the case as this stage.

@metacosm - Sure

csviri · 2021-08-23T07:39:20Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Metrics.java

+import java.util.function.ToDoubleFunction;
+import java.util.function.ToLongFunction;
+
+public class Metrics {


For me this whole construct while wrapping the logic in a Metrics class is little bit smelly, not sure if there is a better way. But will take a look soon.

Initially, I wrote a separate class without wrapping it. @metacosm - Chris asked to have it like above wrapping the logic.

What's wrong with wrapping the logic, @csviri? More than wrapping, it's about measuring things that make sense from the operator's perspective as opposed to calling more metrics-oriented methods. Another aspect is that all metrics code is in a single spot which makes it easier to figure out what's measured and change it if needed.

That the metrics code is in one place that is good. That we are mixing the application logic with metrics is not that nice. Those should be as loosely coupled as possible IMHO. And also seamlessly if possible.

I don't say I have a better answer for this now, just mentioning that design principle. So something to think about.

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Metrics.java

feature: micrometer integration for genering metrics feature: modified variables and made it private feature: added changes for customize increment method feature: removed ConterDetails class and modified IncrementCounter accordingly feature: modified pom depedency feature: modified pom depedency feature: added changes and wrapped counter and timer with createAndUpdate method feature: modified and added inceremenmt() method feature: modified for enabling google formatter removed spaces removed spaces feature: added counter and timer for delete resource feature: added counter and timer for delete resource feature: added changes for delete resource feature: added changes for delete resource feature: update control modified feature: delete control modified feature: delete control modified test delete resource feature: modiofied to retest feature: modified according to PR review comments feature: added count for each and event (create, update, and delete) feature: added count for each and event (create, update, and delete) feature: added Histogram changes for Time feature: added histogram changes for delete as well feature: modified name field in tags feature: retry counter added

jmrodri · 2021-08-25T18:47:26Z

@laxmikantbpandhare can you make your branch writable by maintainers, please? PRs should be opened in a way that maintainers can change stuff on it.

This sounds odd to me. I've never done that for any of my other contributions to other projects.

laxmikantbpandhare · 2021-08-25T18:52:27Z

Update on the PR -

number of controller executions - Added count of controller execution for create, update, and delete events.
number of events processed - Added common count in each try and catch block of createUpdate and Delete method. It will count total events as we discussed in last weeks call.
controller execution durations - Added timer for controller execution which will count method execution time for createUpdate, and Delete method .
number of custom resource managed - As per discussion with @csviri, we are ignoring this task for now.
retry related metrics - Added changes for count retry related metrics.

laxmikantbpandhare · 2021-08-25T18:59:07Z

@metacosm - I added changes in DefaultEventHandler. I created object of Metrics and not added it into the Constructor Parameter as it is causing changes in Multiple files. I used NoopMeterRegistry due to which it asked me to make it Public. These are the changes I did as part of retry related metrics.

...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java

metacosm

Let's get this merged and do subsequent changes on different PRs.

laxmikantbpandhare added work in progres don't merge blocked labels Aug 6, 2021

laxmikantbpandhare requested review from metacosm and csviri August 6, 2021 19:06

laxmikantbpandhare self-assigned this Aug 6, 2021

This was linked to issues Aug 6, 2021

Metrics - Micrometer Integration #64

Closed

Expose Operator metrics out-of-the-box #323

Closed

laxmikantbpandhare changed the title ~~PR for - Micrometer Integration - #64~~ PR for Micrometer Integration - #64 Aug 6, 2021

adam-sandor reviewed Aug 9, 2021

View reviewed changes

laxmikantbpandhare mentioned this pull request Aug 12, 2021

Metrics - Micrometer Integration #64

Closed

laxmikantbpandhare requested a review from adam-sandor August 12, 2021 16:42

metacosm marked this pull request as draft August 16, 2021 16:17

laxmikantbpandhare removed the blocked label Aug 18, 2021

laxmikantbpandhare removed the don't merge label Aug 19, 2021

metacosm reviewed Aug 20, 2021

View reviewed changes

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Metrics.java Outdated Show resolved Hide resolved

metacosm reviewed Aug 20, 2021

View reviewed changes

...tor-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java Outdated Show resolved Hide resolved

metacosm reviewed Aug 20, 2021

View reviewed changes

...tor-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java Outdated Show resolved Hide resolved

metacosm reviewed Aug 20, 2021

View reviewed changes

...framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java Outdated Show resolved Hide resolved

metacosm marked this pull request as draft August 20, 2021 11:01

csviri reviewed Aug 23, 2021

View reviewed changes

metacosm mentioned this pull request Aug 23, 2021

feat: initial PoC for micrometer integration #497

Closed

metacosm reviewed Aug 25, 2021

View reviewed changes

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Metrics.java Outdated Show resolved Hide resolved

laxmikantbpandhare marked this pull request as ready for review August 25, 2021 18:52

laxmikantbpandhare requested review from metacosm, csviri and jmrodri August 25, 2021 18:52

laxmikantbpandhare added 2 commits August 25, 2021 23:08

feature: modified events logic

04ad0bd

comment: removed sysout

6380615

metacosm requested changes Aug 26, 2021

View reviewed changes

...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java Outdated Show resolved Hide resolved

feature: modified for addition of config in defaulteventhandler

34a5dbe

laxmikantbpandhare requested a review from metacosm August 26, 2021 16:12

metacosm approved these changes Aug 26, 2021

View reviewed changes

metacosm merged commit 7aa9bf3 into operator-framework:master Aug 26, 2021

laxmikantbpandhare added kind/feature Categorizes issue or PR as related to a new feature. feature labels Aug 26, 2021

PR for Micrometer Integration - #64 #486

PR for Micrometer Integration - #64 #486

Uh oh!

Conversation

laxmikantbpandhare commented Aug 6, 2021

Uh oh!

jmrodri commented Aug 6, 2021

Uh oh!

jmrodri commented Aug 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laxmikantbpandhare commented Aug 9, 2021

Uh oh!

laxmikantbpandhare commented Aug 11, 2021

Uh oh!

jmrodri commented Aug 11, 2021

Uh oh!

jmrodri commented Aug 11, 2021

Uh oh!

laxmikantbpandhare commented Aug 11, 2021

Uh oh!

jmrodri commented Aug 11, 2021

Uh oh!

laxmikantbpandhare commented Aug 11, 2021

Uh oh!

metacosm commented Aug 16, 2021

Uh oh!

laxmikantbpandhare commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metacosm commented Aug 17, 2021

Uh oh!

laxmikantbpandhare commented Aug 17, 2021

Uh oh!

laxmikantbpandhare commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metacosm commented Aug 18, 2021

Uh oh!

laxmikantbpandhare commented Aug 18, 2021

Uh oh!

laxmikantbpandhare commented Aug 19, 2021

Uh oh!

laxmikantbpandhare commented Aug 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metacosm commented Aug 20, 2021

Uh oh!

laxmikantbpandhare commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmrodri commented Aug 25, 2021

Uh oh!

laxmikantbpandhare commented Aug 25, 2021

Uh oh!

laxmikantbpandhare commented Aug 25, 2021

Uh oh!

laxmikantbpandhare commented Aug 17, 2021 •

edited

Loading

laxmikantbpandhare commented Aug 18, 2021 •

edited

Loading

laxmikantbpandhare commented Aug 20, 2021 •

edited

Loading

csviri Aug 23, 2021 •

edited

Loading