Skip to content

Conversation

@dewabisma
Copy link
Contributor

Changes

  • add os metrics
  • add twitter API metrics

@dewabisma dewabisma requested a review from n13 January 22, 2026 06:18
Copy link
Contributor

@n13 n13 left a comment

Choose a reason for hiding this comment

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

PR #55 Review: Twitter API Metrics

Feedback

1. Histogram bucket at 0.0 is unused

In TWITTER_TWEETS_PER_CALL, the first bucket is 0.0, but track_tweets_pulled only records when tweet_count > 0:

pub fn track_tweets_pulled(operation: &str, tweet_count: usize) {
    if tweet_count > 0 {  // <-- never records 0
        // ...
        TWITTER_TWEETS_PER_CALL.with_label_values(&[operation]).observe(tweet_count as f64);
    }
}

Consider either:

  • Starting buckets at 1.0: vec![1.0, 5.0, 10.0, ...]
  • Or removing the if tweet_count > 0 check to capture zero-result API calls (which could be interesting to track)

2. Generic error type limits observability

The error tracking always uses "api_error":

if result.is_err() {
    TWITTER_API_ERRORS_TOTAL
        .with_label_values(&[operation, "api_error"])
        .inc();
}

Future consideration: extract error types (rate limit, auth, network, etc.) to differentiate failures. Not blocking, but worth noting.

3. Minor: Doc comments added

The new functions have doc comments. While the codebase rules prefer minimal comments, these are public API documentation, which is generally appropriate for helper functions other developers will use.


What looks good

  • Clean abstraction with track_twitter_api_call - wraps any async call elegantly
  • Proper Linux-only conditional compilation for ProcessCollector
  • Type change from i32 to usize for tweet counts is semantically correct
  • Histogram bucket selections are reasonable for the expected distributions
  • No duplicate code - both services use the same tracking functions (follows DRY)

Verdict

Approve with minor suggestions. The code is well-structured and the metrics will provide valuable observability into Twitter API usage. The bucket issue is minor and the error type refinement can be addressed later if needed.

@dewabisma dewabisma merged commit 4bcffa8 into main Jan 22, 2026
1 check passed
@dewabisma dewabisma deleted the fix/metrics-endpoint branch January 22, 2026 07:30
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.

3 participants