fix: prevent exporter panic, harden auth config, improve error handling (#156)

* fix: prevent exporter panic, harden auth config, improve error handling

* feat: set request timeout

* fixed test issue

---------

Co-authored-by: Kostiantyn Kulbachnyi <[email protected]>

Author: bwnuts

internal/github_client/github_client.go

@@ -15,20 +15,29 @@ import (
 	"golang.org/x/oauth2"
 )
 
-func GetRemainingLimits(c *github.Client) RateLimits {
-	ctx := context.Background()
-
-	limits, _, err := c.RateLimit.Get(ctx)
-	if err != nil {
-		utils.RespError(err)
-	}
-
-	return RateLimits{
-		Limit:       limits.Core.Limit,
-		Remaining:   limits.Core.Remaining,
-		Used:        limits.Core.Limit - limits.Core.Remaining,
-		SecondsLeft: time.Until(limits.Core.Reset.Time).Seconds(),
-	}
+func GetRemainingLimits(c *github.Client) (RateLimits, error) {
+    if c == nil {
+        return RateLimits{}, utils.RespError(fmt.Errorf("github client is nil"))
+    }
+
+    // Set a timeout of 5 seconds for the request
+    ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+    defer cancel()
+
+    limits, _, err := c.RateLimit.Get(ctx)
+    if err != nil {
+        return RateLimits{}, utils.RespError(err)
+    }
+    if limits == nil || limits.Core == nil {
+        return RateLimits{}, utils.RespError(fmt.Errorf("rate limit response is nil"))
+    }
+
+    return RateLimits{
+        Limit:       limits.Core.Limit,
+        Remaining:   limits.Core.Remaining,
+        Used:        limits.Core.Limit - limits.Core.Remaining,
+        SecondsLeft: time.Until(limits.Core.Reset.Time).Seconds(),
+    }, nil
 }
 
 func (c *TokenConfig) InitClient() *github.Client {
@@ -57,11 +66,18 @@ func InitConfig() GithubClient {
 			installationID, _ = strconv.ParseInt(envInstallationID, 10, 64)
 		}
 
+		// Only read organization/repo names if InstallationID is not provided
+		var orgName, repoName string
+		if envInstallationID == "" {
+			orgName = utils.GetOSVar("GITHUB_ORG_NAME")
+			repoName = utils.GetOSVar("GITHUB_REPO_NAME")
+		}
+
 		auth = &AppConfig{
 			AppID:          appID,
 			InstallationID: installationID,
-			OrgName:        utils.GetOSVar("GITHUB_ORG_NAME"),
-			RepoName:       utils.GetOSVar("GITHUB_REPO_NAME"),
+			OrgName:        orgName,
+			RepoName:       repoName,
 			PrivateKeyPath: utils.GetOSVar("GITHUB_PRIVATE_KEY_PATH"),
 		}
 	} else {

internal/github_client/github_client_test.go

@@ -72,7 +72,8 @@ func TestGetRemainingLimits(t *testing.T) {
 		),
 	)
 	c := github.NewClient(mockedHTTPClient)
-	limits := GetRemainingLimits(c)
+    limits, err := GetRemainingLimits(c)
+    assert.NoError(t, err)
 
 	assert.Equal(t, limit, limits.Limit, "The limits should be equal")
 	assert.Equal(t, remaining, limits.Remaining, "The remaining limits should be equal")

internal/prometheus_exporter/server.go

@@ -49,30 +49,40 @@ func (collector *LimitsCollector) Describe(ch chan<- *prometheus.Desc) {
 	ch <- collector.SecondsLeft
 }
 
-func (collector *LimitsCollector) Collect(ch chan<- prometheus.Metric) {
-
-	auth := github_client.InitConfig()
-	limits := github_client.GetRemainingLimits(auth.InitClient())
-	if logMetricCollection {
-		log.Printf("Collected metrics for %s", githubAccount)
-		log.Printf("Limit: %d | Used: %d | Remaining: %d", limits.Limit, limits.Used, limits.Remaining)
-	}
-	//Write latest value for each metric in the prometheus metric channel.
-	//Note that you can pass CounterValue, GaugeValue, or UntypedValue types here.
+func (collector *LimitsCollector) emitMetrics(ch chan<- prometheus.Metric, limits github_client.RateLimits) {
 	m1 := prometheus.MustNewConstMetric(collector.LimitTotal, prometheus.GaugeValue, float64(limits.Limit))
 	m2 := prometheus.MustNewConstMetric(collector.LimitRemaining, prometheus.GaugeValue, float64(limits.Remaining))
 	m3 := prometheus.MustNewConstMetric(collector.LimitUsed, prometheus.GaugeValue, float64(limits.Used))
 	m4 := prometheus.MustNewConstMetric(collector.SecondsLeft, prometheus.GaugeValue, limits.SecondsLeft)
-	m1 = prometheus.NewMetricWithTimestamp(time.Now(), m1)
-	m2 = prometheus.NewMetricWithTimestamp(time.Now(), m2)
-	m3 = prometheus.NewMetricWithTimestamp(time.Now(), m3)
-	m4 = prometheus.NewMetricWithTimestamp(time.Now(), m4)
+	now := time.Now()
+	m1 = prometheus.NewMetricWithTimestamp(now, m1)
+	m2 = prometheus.NewMetricWithTimestamp(now, m2)
+	m3 = prometheus.NewMetricWithTimestamp(now, m3)
+	m4 = prometheus.NewMetricWithTimestamp(now, m4)
 	ch <- m1
 	ch <- m2
 	ch <- m3
 	ch <- m4
 }
 
+func (collector *LimitsCollector) Collect(ch chan<- prometheus.Metric) {
+	auth := github_client.InitConfig()
+	limits, err := github_client.GetRemainingLimits(auth.InitClient())
+	if err != nil {
+		// On error expose zeros and return to avoid panics during scraping
+		if logMetricCollection {
+			log.Printf("error collecting metrics for %s: %v", githubAccount, err)
+		}
+		collector.emitMetrics(ch, github_client.RateLimits{})
+		return
+	}
+	if logMetricCollection {
+		log.Printf("Collected metrics for %s", githubAccount)
+		log.Printf("Limit: %d | Used: %d | Remaining: %d", limits.Limit, limits.Used, limits.Remaining)
+	}
+	collector.emitMetrics(ch, limits)
+}
+
 func Run() {
 	// Default to logging metric collection
 	if logMetricCollectionParseErr != nil {

internal/utils/utils.go

@@ -3,12 +3,16 @@ package utils
 import (
 	"errors"
 	"fmt"
+	"log"
 	"os"
 )
 
+// RespError logs and returns a wrapped error for callers to handle.
+// This function does not terminate the process.
 func RespError(err error) error {
 	if err != nil {
-		errMsg := fmt.Sprintf("there was an error during the call execution: %s\n", err)
+		errMsg := fmt.Sprintf("there was an error during the call execution: %s", err)
+		log.Printf("%s", errMsg)
 		return errors.New(errMsg)
 	}
 	return nil
@@ -18,6 +22,7 @@ func GetOSVar(envVar string) string {
 	value, present := os.LookupEnv(envVar)
 	if !present {
 		err := fmt.Sprintf("environment variable %s not set", envVar)
+		// Log the error but do not terminate; caller should validate the value.
 		RespError(errors.New(err))
 		return ""
 	}

internal/utils/utils_test.go

@@ -24,5 +24,5 @@ func TestGetOSVar(t *testing.T) {
 
 func TestRespError(t *testing.T) {
 	err := errors.New("test")
-	assert.Equal(t, errors.New("there was an error during the call execution: test\n"), RespError(err))
+	assert.Equal(t, errors.New("there was an error during the call execution: test"), RespError(err))
 }