Skip to content

feat: api transport metrics #4774

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: api transport metrics #4774

wants to merge 3 commits into from

Conversation

acpana
Copy link
Collaborator

@acpana acpana commented Jul 10, 2025 •
edited
Loading

Patch adds transport level API metrics for HTTP calls that KCC makes.

Follow up work:

  • plumb controller_name for TF and DCL resources
  • automated smoke testing for metrics existence
  • plumb through for KCC manager

Testing

  • Enable metrics
  • add a LoggingLogMetric resource
  • observe:
$ curl http://localhost:8888/metrics2 | grep "gcp_api_requests_total"
...
# HELP configconnector_gcp_api_requests_total Total number of GCP API requests
# TYPE configconnector_gcp_api_requests_total counter
 configconnector_gcp_api_requests_total{controller_name="logginglogmetric-controller",method="GET",service="logging",status_code="200"} 1
 configconnector_gcp_api_requests_total{controller_name="logginglogmetric-controller",method="PUT",service="logging",status_code="200"} 1
...

acpana added 2 commits July 10, 2025 23:30
@acpana
feat: transport metrics
25fc521 
Signed-off-by: Alex Pana <[email protected]>
@acpana
feat:metrics: direct llm
6640c1f 
Signed-off-by: Alex Pana <[email protected]>
@google-oss-prow Google OSS Prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cheftako for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@acpana
Copy link
Collaborator Author

acpana commented Jul 11, 2025

the current set up breaks the tests so it still needs some work!

@acpana
refactor: test config
72f632d 
Signed-off-by: Alex Pana <[email protected]>
@acpana acpana marked this pull request as ready for review July 17, 2025 23:58
cheftako
cheftako reviewed Jul 18, 2025
View reviewed changes
pkg/config/controllerconfig.go
"golang.org/x/oauth2"
"google.golang.org/api/option"
htransport "google.golang.org/api/transport/http"
Copy link
Collaborator

Choose a reason for hiding this comment

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

httptransport?

cheftako
cheftako reviewed Jul 18, 2025
View reviewed changes
pkg/config/controllerconfig.go
return c, err
}

// Create an authenticated transport using htransport
Copy link
Collaborator

@cheftako cheftako Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This comment does not seem to align with the code in the immediate block.

cheftako
cheftako reviewed Jul 18, 2025
View reviewed changes
pkg/metrics/register.go
@@ -45,7 +46,8 @@ func RegisterPrometheusExporter(addr string) error {
// Run the Prometheus exporter as a scrape endpoint.
go func() {
mux := http.NewServeMux()
mux.Handle("/metrics", pe)
mux.Handle("/metrics", pe) // OpenCensus
mux.Handle("/experimental-metrics", promhttp.Handler()) // Prometheus Go client
Copy link
Collaborator

Choose a reason for hiding this comment

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

I grant its experimental right now but not sure that the path we want to use. How about /prom-metrics or /prometheus-metrics ?

cheftako
cheftako reviewed Jul 18, 2025
View reviewed changes
pkg/metrics/transport/transport.go

statusCode := "0" // good to have a default value here
if resp != nil {
statusCode = strconv.Itoa(resp.StatusCode)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not bounded.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should be going through bounded types and then converting to string at the WithLabelValues call to ensure the labels are actually bounded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheftako cheftako cheftako left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@acpana @cheftako