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

add prom metrics to frontend #54

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Conversation

katherinelc321
Copy link
Collaborator

@katherinelc321 katherinelc321 commented Apr 15, 2024

What this PR does

Before this PR:
Frontend has no monitoring
After this PR:
Added prometheus metrics collector and init
Jira: https://issues.redhat.com/browse/ARO-6284

Special notes for your reviewer

curling /metrics shows this:

curl -X GET 0.0.0.0:8443/metrics
# HELP frontend_count 
# TYPE frontend_count counter
frontend_count{api_version="",code="200",route="/healthz/ready",verb="GET"} 1
frontend_count{api_version="",code="200",route="/metrics",verb="GET"} 2
# HELP frontend_duration 
# TYPE frontend_duration gauge
frontend_duration{api_version="",code="200",route="/healthz/ready",verb="GET"} 0
frontend_duration{api_version="",code="200",route="/metrics",verb="GET"} 0
# HELP frontend_health 
# TYPE frontend_health gauge
frontend_health{endpoint="/healthz/ready"} 1

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

  • PR: The PR description is expressive enough and will help future contributors
  • Code: Write code that humans can understand and Keep it simple
  • Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
  • Upgrade: Impact of this change on upgrade flows was considered and addressed if required
  • Deployment: The deployment process was considered and addressed if required
  • Testing: New code requires new unit tests.
  • Documentation: Is the documentation updated? Either in the doc located in focus area, in the README or in the code itself.
  • Customers: Is this change affecting customers? Is the release plan considered?

Copy link
Collaborator

@mbarnes mbarnes left a comment

Choose a reason for hiding this comment

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

Small change requested to eliminate a redundant constant.

internal/api/registry.go Outdated Show resolved Hide resolved
frontend/metrics.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bennerv bennerv left a comment

Choose a reason for hiding this comment

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

one comment, otherwise lgtm

frontend/go.mod Outdated Show resolved Hide resolved
frontend/metrics.go Outdated Show resolved Hide resolved
Copy link

@AldoFusterTurpin AldoFusterTurpin left a comment

Choose a reason for hiding this comment

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

Thank you for this. 🙂

Just a couple of questions about why not to use the metrics.Emitter interface when possible in the parameters and a question about (*PrometheusEmitter).EmitGauge... syntax.

Thanks!

frontend/frontend.go Outdated Show resolved Hide resolved
frontend/frontend.go Outdated Show resolved Hide resolved
frontend/frontend.go Outdated Show resolved Hide resolved
frontend/metrics.go Outdated Show resolved Hide resolved
frontend/metrics.go Outdated Show resolved Hide resolved
frontend/frontend.go Outdated Show resolved Hide resolved
@katherinelc321 katherinelc321 force-pushed the aro-6284 branch 2 times, most recently from b2a031d to ef7b7bb Compare April 18, 2024 18:49
frontend/frontend.go Outdated Show resolved Hide resolved
s-amann
s-amann previously approved these changes Apr 22, 2024
mbarnes
mbarnes previously approved these changes Apr 23, 2024
@katherinelc321 katherinelc321 dismissed stale reviews from mbarnes and s-amann via 06f76c5 April 23, 2024 14:15
@mbarnes
Copy link
Collaborator

mbarnes commented Apr 23, 2024

It looks like you addressed all of @AldoFusterTurpin's requested changes, so I'm going to dismiss his review in order to unblock this.

@mbarnes mbarnes dismissed AldoFusterTurpin’s stale review April 23, 2024 15:36

Requested changes have been addressed.

@mbarnes mbarnes merged commit d5615c4 into Azure:main Apr 23, 2024
3 checks passed
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.

5 participants