Skip to content

Conversation

amanfcp
Copy link
Contributor

@amanfcp amanfcp commented Sep 18, 2025

  1. Increment counter for URL
  2. Time the latency
  3. Increment counter for non-200 calls
  4. Added Tests

Note: Claude code helped me in writing tests.

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

1. Increment counter for URL
2. Time the latency
3. Increment counter for non-200 calls
Added Tests
@amanfcp amanfcp requested a review from a team as a code owner September 18, 2025 08:46
Comment on lines +74 to +77
// Limit path length to avoid extremely long paths creating high cardinality
if len(sanitized.Path) > 100 {
sanitized.Path = sanitized.Path[:100] + "..."
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need suggestions, if we need to do this trimming or not

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 I think this is good. ...Pretty sure other metrics systems do this by default too (though it's been a while since I used like, Datadog)

Copy link
Contributor

@camgunz camgunz left a comment

Choose a reason for hiding this comment

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

LG! Thanks for the quick turnaround 🙏🏻

prometheus.CounterOpts{
Namespace: MetricsNamespace,
Subsystem: "http_client",
Name: "requests_total",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's prefix all these w/ http_

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, if Subsystem up there already namespaces it, then this is fine 👌🏻 (I am not a prometheus expert haha)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Namespace, Subsystem, and Name are components of the fully-qualified

As per this doc, subsystem is already prefixing the metric. So I guess the metric title would look something like trufflehog_http_client_requests_total

Copy link
Contributor

Choose a reason for hiding this comment

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

OK that's perfect 👍🏻

Comment on lines +74 to +77
// Limit path length to avoid extremely long paths creating high cardinality
if len(sanitized.Path) > 100 {
sanitized.Path = sanitized.Path[:100] + "..."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 I think this is good. ...Pretty sure other metrics systems do this by default too (though it's been a while since I used like, Datadog)

@amanfcp amanfcp merged commit 9adec3c into main Sep 18, 2025
13 checks passed
@amanfcp amanfcp deleted the INS-51/add_metrics_to_sanehttpclient branch September 18, 2025 12:25
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.

2 participants