Refactor Sentry error reporting pattern with flexible options across application layers by 0xaboomar · Pull Request #62 · OneBusAway/watchdog

0xaboomar · 2025-06-15T00:02:25Z

Refactored Sentry reporting to use a unified SentryReportOptions for passing tags, context, and severity level
Introduced ReportErrorWithSentryOptions to replace ad-hoc reporting calls with a consistent helper
Improved ConfigureScope to include goarch and renamed version tag to app_version
Renamed ReportIfProd → ReportError
Updated main application flow to consistently use the Reporter with the new pattern (GTFS download, config loading, cache creation)
Integrated Sentry reporting into collectMetricsForServer
Enhanced tests in main_test.go for Reporter integration

- Added `SentryReportOptions` struct to allow passing extra context, custom tags, and severity level. - Introduced `ReportErrorWithSentryOptions` method for reporting with fine-grained control. - Improved `ConfigureScope` by including `goarch` and renaming `version` tag to `app_version`. - Renamed `ReportIfProd` to `ReportError` and removed environment check to allow caller control.

…pplication flow - Add Reporter dependency to application struct and main functions - Report errors to Sentry with tags and context across GTFS download, config loading, and cache directory creation - Refactor main.go to pass Reporter to functions that can produce reportable errors

…rServer

coveralls · 2025-06-15T00:04:57Z

coverage: 67.407% (-3.4%) from 70.833%
when pulling a1bf3eb on Abo-Omar-74:main
into e5a84f4 on OneBusAway:main.

Copilot

Pull Request Overview

This PR refactors how Sentry error reporting is handled by introducing a unified options struct and updating all layers to use a consistent reporting helper.

Introduces SentryReportOptions and ReportErrorWithSentryOptions
Renames ReportIfProd → ReportError and standardizes Sentry scope tags (e.g., app_version, goarch)
Injects Reporter into main, metrics, and test code and updates function signatures accordingly

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
internal/report/reporter.go	Added `SentryReportOptions` type, `ReportErrorWithSentryOptions`, updated `ReportError` and scope configuration
cmd/watchdog/metrics.go	Replaced ad-hoc Sentry calls with `ReportErrorWithSentryOptions` in `collectMetricsForServer`
cmd/watchdog/main_test.go	Injected `Reporter` into tests and updated calls to loader functions
cmd/watchdog/main.go	Wired reporter through application, updated function signatures, set up and flushed Sentry

Comments suppressed due to low confidence (2)

internal/report/reporter.go:7

This file uses runtime.Version(), runtime.GOARCH, and os.Hostname(), but neither runtime nor os are imported. Add "runtime" and "os" to the import block to avoid compile errors.

import (

internal/report/reporter.go:70

Consider adding unit tests for ReportError and ReportErrorWithSentryOptions to verify that tags, context, and default level behaviors are applied correctly.

type SentryReportOptions struct {

Copilot · 2025-06-15T21:15:22Z

+		if opts.Level != "" {
+			scope.SetLevel(opts.Level)
+		}


[nitpick] If no level is provided (opts.Level is zero), no level is set on the scope. Consider defaulting opts.Level to sentry.LevelError before this check so every reported error has an explicit severity.

Suggested change

if opts.Level != "" {

scope.SetLevel(opts.Level)

}

level := opts.Level

if level == "" {

level = sentry.LevelError

}

scope.SetLevel(level)

Copilot · 2025-06-15T21:15:22Z

 		tmpFile.Close()

-		servers, err := loadConfigFromFile(tmpFile.Name())
+		servers, err := loadConfigFromFile(tmpFile.Name() , reporter)


[nitpick] There’s a stray space before the comma in the function call. Removing it (tmpFile.Name(), reporter) will keep the code consistent with standard Go formatting.

Suggested change

servers, err := loadConfigFromFile(tmpFile.Name() , reporter)

servers, err := loadConfigFromFile(tmpFile.Name(), reporter)

aaronbrethorst

code looks good! I have some nitpicks I'd like to see addressed before merging, but this is good work! 👍

aaronbrethorst · 2025-06-15T21:23:20Z


 	cacheDir := "cache"
-	if err = createCacheDirectory(cacheDir, logger); err != nil {
+	if err = createCacheDirectory(cacheDir, logger , reporter); err != nil {


there's an extra space in between logger and the comma. it should look like this: logger, reporter

aaronbrethorst · 2025-06-15T21:23:25Z


 	// Download GTFS bundles for all servers on startup
-	downloadGTFSBundles(servers, cacheDir, logger)
+	downloadGTFSBundles(servers, cacheDir, logger , reporter)


there's an extra space in between logger and the comma. it should look like this: logger, reporter.

aaronbrethorst · 2025-06-15T21:23:33Z


 	// Cron job to download GTFS bundles for all servers every 24 hours
-	go refreshGTFSBundles(servers, cacheDir, logger , 24 * time.Hour)
+	go refreshGTFSBundles(servers, cacheDir, logger , 24 * time.Hour , reporter)


there's an extra space in between logger and the comma. it should look like this: logger, reporter

aaronbrethorst · 2025-06-15T21:25:30Z

 	if err != nil {
 		if os.IsNotExist(err){
 			if err := os.MkdirAll(cacheDir, os.ModePerm); err != nil {
+				reporter.ReportErrorWithSentryOptions(err, report.SentryReportOptions{


I think you need to run go fmt on this code. the indentation is strange.

aaronbrethorst · 2025-06-15T21:25:55Z

 	}
 	if !stat.IsDir() {
-		return fmt.Errorf("%s is not a directory", cacheDir)
+		err := fmt.Errorf("%s is not a directory", cacheDir)


same here on formatting.

aaronbrethorst · 2025-06-15T21:26:29Z

 		_, err := utils.DownloadGTFSBundle(server.GtfsUrl, cacheDir, server.ID, hashStr)
 		if err != nil {
+			reporter.ReportErrorWithSentryOptions(err, report.SentryReportOptions{
+			Tags: map[string]string{


indentation

aaronbrethorst · 2025-06-15T21:26:47Z


 // refreshGTFSBundles periodically downloads GTFS bundles at the specified interval.
-func refreshGTFSBundles(servers []models.ObaServer, cacheDir string, logger *slog.Logger , interval time.Duration) {
+func refreshGTFSBundles(servers []models.ObaServer, cacheDir string, logger *slog.Logger , interval time.Duration , reporter *report.Reporter) {


watch those extra spaces again ,, here and throughout the function below

aaronbrethorst · 2025-06-15T21:30:51Z

+		newServers, err := loadConfigFromURL(configURL, configAuthUser, configAuthPass , reporter)
 		if err != nil {
+			reporter.ReportErrorWithSentryOptions(err, report.SentryReportOptions{
+				Tags: map[string]string{


you're creating these map[string]string types a lot here for maps with a single key/value pair. I recommend creating a helper function that can generate them for you. it'll make the code more readable:

func makeMap(key, value string) map[string]string { return map[string]string{ key: value } }

aaronbrethorst · 2025-06-15T21:31:04Z

+func loadConfigFromFile(filePath string , reporter *report.Reporter) ([]models.ObaServer, error) {
 	data, err := os.ReadFile(filePath)
 	if err != nil {
+		reporter.ReportErrorWithSentryOptions(err, report.SentryReportOptions{


aaronbrethorst · 2025-06-15T21:31:10Z


 	var servers []models.ObaServer
 	if err := json.Unmarshal(data, &servers); err != nil {
+		reporter.ReportErrorWithSentryOptions(err, report.SentryReportOptions{


aaronbrethorst

Lgtm!

0xaboomar added 4 commits June 15, 2025 02:48

test(main): integrate reporter with main_test.go

123890a

feat(metrics): integrate Sentry error reporting into collectMetricsFo…

60e0e2b

…rServer

aaronbrethorst requested a review from Copilot June 15, 2025 21:14

Copilot AI reviewed Jun 15, 2025

View reviewed changes

aaronbrethorst requested changes Jun 15, 2025

View reviewed changes

0xaboomar added 4 commits June 16, 2025 01:06

style: fix indentation, remove stray and extra spaces (ran go fmt)

4c61601

feat(utils): add MakeMap helper for map[string]string creation

834bb2f

refactor(main): use MakeMap util for map creation

ee84715

docs(utils): add documentation for MakeMap helper function

a1bf3eb

0xaboomar force-pushed the main branch from 1e7a78b to a1bf3eb Compare June 16, 2025 02:21

aaronbrethorst approved these changes Jun 17, 2025

View reviewed changes

aaronbrethorst merged commit 578749c into OneBusAway:main Jun 17, 2025
4 checks passed

-		if opts.Level != "" {
-			scope.SetLevel(opts.Level)
-		}
+		level := opts.Level
+		if level == "" {
+			level = sentry.LevelError
+		}
+		scope.SetLevel(level)

	servers, err := loadConfigFromFile(tmpFile.Name() , reporter)
	servers, err := loadConfigFromFile(tmpFile.Name(), reporter)

Conversation

0xaboomar commented Jun 15, 2025

Uh oh!

coveralls commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 15, 2025

Choose a reason for hiding this comment

Uh oh!

aaronbrethorst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronbrethorst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jun 15, 2025 •

edited

Loading