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

Send registry image pulls metrics to prometheus #1238

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

otaviojacobi
Copy link
Contributor

Change-type: patch


export function incrementRegistryImagePulls(
value: number = 1,
payload: any = {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you type this better than any please

);

export function incrementRegistryImagePulls(
value: number = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

When you specify a default value then the type defaults to the type of that default value:

Suggested change
value: number = 1,
value = 1,

There's nothing wrong with explicitly specifying number but it's redundant and makes sense imo to reserve for only the cases where you want a type that is different to that of the default value

@@ -591,6 +592,17 @@ export const token: RequestHandler = async (req, res) => {
authorizeRequest(req, scopes, tx),
]),
);

access.forEach((ac) =>
incrementRegistryImagePulls(1, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would name the metric differently to show that it is images that have had a token validated for access rather than image pulls (you can use the image to pull 0 or 2+ times as well so it's not 1-1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants