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

feat(agent):metrics for errors in agent #3449

Merged
4 commits merged into from Oct 24, 2022
Merged

feat(agent):metrics for errors in agent #3449

4 commits merged into from Oct 24, 2022

Conversation

ghost
Copy link

@ghost ghost commented Oct 22, 2022

Checklist

  • I have read the coding guide.
  • I have filled out the description and linked the related issues.

Description

Adds metrics for errors for Grafana dashboard

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

An extension of Agent feature in storage incentives.

Related Issue (Optional)

NA

Screenshots (if appropriate):


This change is Reviewable

@ghost ghost requested a review from istae October 22, 2022 22:01
@ghost ghost requested a review from notanatol October 22, 2022 22:12
@ghost ghost self-assigned this Oct 22, 2022
@ghost ghost requested a review from vladopajic October 24, 2022 08:09
@ghost
Copy link
Author

ghost commented Oct 24, 2022

@istae @vladopajic ci errs unrelated to pr

Copy link
Member

@istae istae left a comment

Choose a reason for hiding this comment

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

Nice work, small suggestions :)

pkg/storageincentives/agent.go Outdated Show resolved Hide resolved
pkg/storageincentives/agent.go Show resolved Hide resolved
pkg/storageincentives/agent.go Outdated Show resolved Hide resolved
pkg/storageincentives/metrics.go Outdated Show resolved Hide resolved
pkg/storageincentives/agent.go Outdated Show resolved Hide resolved
pkg/storageincentives/agent.go Show resolved Hide resolved
@ghost ghost requested a review from istae October 24, 2022 08:59
ErrCheckIsPlaying: prometheus.NewCounter(prometheus.CounterOpts{
Namespace: m.Namespace,
Subsystem: subsystem,
Name: "neighborhoodSelected_errors",
Copy link
Member

Choose a reason for hiding this comment

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

rename to is_playing_errors

pkg/storageincentives/agent.go Outdated Show resolved Hide resolved
Comment on lines +23 to +25
// total calls to chain backend
BackendCalls prometheus.Counter
BackendErrors prometheus.Counter
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally these should be exposed by ChainBackend interface (or any other type wrapping this interface in order to provide this this functionality.

@vladopajic
Copy link
Contributor

Ideally metrics should be defined and calculated inside the same scope where logic (which is being measured) is defined.

That is, if measured function is doTask() it would be the best to put metrics inside this function

func doTask() (err error) {
	a.metrics.DoTaskCalls().Inc()
	defer func() {
		if err {
			a.metrcis.DoCallsErr().inc()
		}
	}()

	// ... 
}

Having this practice enables more robust code because code using doTask() function will not have to take care of calculating it's metrics; also it improves code reuse. For example BackendCalls should be measured inside this type, and if it is implemented this way then all other logic would have these metrics out of the box.

@ghost ghost requested a review from istae October 24, 2022 09:45
@ghost
Copy link
Author

ghost commented Oct 24, 2022

Ideally metrics should be defined and calculated inside the same scope where logic (which is being measured) is defined.

That is, if measured function is doTask() it would be the best to put metrics inside this function

func doTask() (err error) {
	a.metrics.DoTaskCalls().Inc()
	defer func() {
		if err {
			a.metrcis.DoCallsErr().inc()
		}
	}()

	// ... 
}

Having this practice enables more robust code because code using doTask() function will not have to take care of calculating it's metrics; also it improves code reuse. For example BackendCalls should be measured inside this type, and if it is implemented this way then all other logic would have these metrics out of the box.

makes sense, thankyou, will keep it in mind.

@ghost ghost merged commit 00aada8 into master Oct 24, 2022
@ghost ghost deleted the agent-metrics branch October 24, 2022 10:07
@aloknerurkar aloknerurkar added this to the 1.10.0 milestone Dec 8, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants