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

reporting repository provision request times to prometheus for SLO/SLI #84

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

rcerven
Copy link
Contributor

@rcerven rcerven commented Dec 21, 2023

@rcerven rcerven changed the title reporting request times for each action to prometheus for SLO/SLI reporting repository provision request times to prometheus for SLO/SLI Dec 21, 2023
@rcerven rcerven requested review from tkdchen, mkosiarc and chmeliik and removed request for psturc December 21, 2023 22:12
controllers/component_image_controller.go Outdated Show resolved Hide resolved
controllers/imagerepository_controller.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
controllers/imagerepository_controller.go Outdated Show resolved Hide resolved
controllers/imagerepository_controller.go Outdated Show resolved Hide resolved
imageRepositoryProvisionTimeMetric.Observe(time.Since(provisionTime).Seconds())
}
// remove component from metrics map
delete(repositoryTimesForMetrics, componentIdForMetrics)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can do the deletion only if the time was recorded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

time will be always recorded at that point

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then why do we need that if above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need real time for observe, I don't have to check that something is in map when removing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean in this case I guess I can just move that delete inside if

controllers/imagerepository_controller.go Outdated Show resolved Hide resolved
@rcerven rcerven force-pushed the slo_br branch 2 times, most recently from 769fb70 to c3d7e41 Compare January 9, 2024 19:03

func getComponentIdForMetrics(component *appstudioredhatcomv1alpha1.Component) string {
return component.Name + "=" + component.Namespace
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is duplicate code at line https://github.com/redhat-appstudio/image-controller/pull/84/files#diff-922fcbfbe2565443329918a2b0d49b2b03588007ec6464a20d3feb824f3f80a2R143

I'd suggest to simplify the arguments and make them explicit

func getComponentIdForMetrics(componentName, componentNS string) string {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well that uses different thing and not component

Copy link
Contributor

@tkdchen tkdchen left a comment

Choose a reason for hiding this comment

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

Function initMetrics is called twice inside both reconciler SetupWithManager functions. I assume the duplicated calls should not cause any potential problem due to the underlying package handles that. Even though, how about to avoid duplicated call explicitly?

Metrics related code are mainly implemented inside imagerepository_controller.go file and shared with ComponentReconciler. My suggestion is to make a separate go file under package controllers to include those shared code related to metrics.

@@ -90,7 +151,12 @@ func (r *ImageRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Requ
return ctrl.Result{}, err
}

componentIdForMetrics := fmt.Sprintf("%s=%s", imageRepository.Labels[ComponentNameLabelName], imageRepository.Namespace)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ImageRepository doesn't have to have ComponentNameLabelName label. For example, JBS uses it without the label.
I think we have to have identifier not related to Component, but to instance of ImageRepository itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will use name + namespace then

@rcerven
Copy link
Contributor Author

rcerven commented Jan 10, 2024

Function initMetrics is called twice inside both reconciler SetupWithManager functions. I assume the duplicated calls should not cause any potential problem due to the underlying package handles that. Even though, how about to avoid duplicated call explicitly?

Metrics related code are mainly implemented inside imagerepository_controller.go file and shared with ComponentReconciler. My suggestion is to make a separate go file under package controllers to include those shared code related to metrics.

did you see the first thing I do in initMetrics??

// don't register it if it was already registered by another controller
if imageRepositoryProvisionTimeMetric != nil {
	return nil
}

that ensures it doesn't run multiple times

but I have to call it in both controllers because 1 of them will be first to initialize it

@rcerven rcerven force-pushed the slo_br branch 2 times, most recently from 100872f to 3704627 Compare January 10, 2024 14:12
Copy link

sonarcloud bot commented Jan 10, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.8% Duplication on New Code

See analysis details on SonarCloud

@rcerven
Copy link
Contributor Author

rcerven commented Jan 10, 2024

/retest

@rcerven rcerven merged commit 7374765 into konflux-ci:main Jan 10, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants