apptracker: Error tracking interface and it's Sentry implementation #34

gouthamp-stellar · 2024-08-11T20:57:43Z

What

Error tracking interface and it's Sentry implementation

Task: https://app.asana.com/0/1207947010297920/1207245825676510

Why

Adding sentry tracking to the Wallet Backend

Known limitations

N/A

Issue that this PR addresses

https://app.asana.com/0/1207947010297920/1207245825676510

Checklist

PR Structure

It is not possible to break this PR down into smaller PRs.
This PR does not mix refactoring changes with feature changes.
This PR's title starts with name of package that is most changed in the PR, or all if the changes are broad or impact many packages.

Thoroughness

This PR adds tests for the new functionality or fixes.
All updated queries have been tested (refer to this check if the data set returned by the updated query is expected to be same as the original one).

Release

This is not a breaking change.
This is ready to be tested in development.
The new functionality is gated with a feature flag if this is not ready for production.

Error tracking interface and it's Sentry implementation Task: https://app.asana.com/0/1207947010297920/1207245825676510

cmd/ingest.go

go mod tidy

lint fixes

CaioTeixeira95

Thank you for working on this. I've left a few comments.

CaioTeixeira95 · 2024-08-16T15:02:26Z

cmd/utils/global_options.go

+	}
+}
+
+func StellarEnvironmentOption(configKey *string) *config.ConfigOption {


What's this environment? What's used for?

Sentry needs a variable called Environment when you call sentry.Init(...) and this basically sets the value for that

It is used to set up the sentry environment which is a variable we need to initialize the sentry tracker

I think we can use the Network Passphrase for that, what do you think?

Seems like we use the sentry environment variable to initialize the tracker in the Vibrant backend so I did the same here

@gouthamp-stellar this is something we're refactoring on the Vibrant backend. Unfortunately we have this duplicate ENV there.
Wdyt about using the Network Passphrase env that we already use for ingestion (like @CaioTeixeira95 suggested) and mapping it to testnet/development or mainnet/production in the NewSentryTracker function.
That way we don't have this duplicate config which at the end of the day means the same thing.

If not maybe we can at least name the env something like TRACKER_ENVIRONMENT so it's not misused in other places interchangeably with NETWORK_PASSPHRASE.

Down to do that in a future pr. I already pushed a pr to kube-vibrant with this env variable. Since it's only a couple lines, I can add this change to an upcoming pr

CaioTeixeira95 · 2024-08-16T15:07:27Z

internal/apptracker/app_tracker.go

Can we add a DryRunAppTracker? This would only log on the Stdout, this is useful for local development environments, so we don't need to use a sentry account.

makes sense. done

internal/apptracker/sentry/sentry_tracker.go

CaioTeixeira95 · 2024-08-16T17:23:20Z

internal/ingest/ingest.go

+	AppTracker           sentry.SentryTracker
+	SentryDsn            string
+	StellarEnvironment   string


I'd suggest we only have the app tracker here. Then on the config we only use a local variable:

var sentryDSN string utils.SentryOptions(&sentryDSN)

internal/serve/middleware/middleware_test.go

CaioTeixeira95 · 2024-08-16T17:41:50Z

internal/serve/middleware/middleware_test.go

@@ -151,6 +154,7 @@ func TestRecoverHandler(t *testing.T) {
 	req, err := http.NewRequest("GET", "/", nil)
 	require.NoError(t, err)
 	rr := httptest.NewRecorder()
+	appTrackerMock.On("CaptureException", mock.Anything).Return(nil)


The same suggestion that I've made on the InternalServerError, can we test the error value here?

internal/serve/serve.go

CaioTeixeira95 · 2024-08-16T17:43:32Z

internal/apptracker/sentry/sentry_tracker.go

+func (s *SentryTracker) CaptureMessage(message string) {
+	sentry.CaptureMessage(message)
+}
+
+func (s *SentryTracker) CaptureException(exception error) {
+	sentry.CaptureException(exception)
+}


Can we add tests for these methods?

cmd/ingest.go

go mod tidy

CaioTeixeira95

Overall, it looks good to me. Please solve the remaining commentaries before merging it.

CaioTeixeira95 · 2024-08-27T17:07:22Z

cmd/ingest.go

+			}
+			appTracker, err := sentry.NewSentryTracker(sentryDSN, stellarEnvironment, 5)
+			if err != nil {
+				log.Fatalf("Error initializing App Tracker: %s", err.Error())


Suggested change

log.Fatalf("Error initializing App Tracker: %s", err.Error())

return fmt.Errorf("initializing app tracker: %w", err)

CaioTeixeira95 · 2024-08-27T17:08:01Z

cmd/ingest.go

+			if err != nil {
+				log.Fatalf("Error initializing App Tracker: %s", err.Error())
+			}
+			cfg.AppTracker = *appTracker


Here you should pass the reference because it is an interface!

I made it
cfg.AppTracker = appTracker

CaioTeixeira95 · 2024-08-27T17:08:33Z

cmd/serve.go

+			if err != nil {
+				return fmt.Errorf("initializing App Tracker: %w", err)
+			}
+			cfg.AppTracker = *appTracker


Here you should pass the reference because it's an interface!

I made it
cfg.AppTracker = appTracker

CaioTeixeira95 · 2024-08-27T17:10:10Z

internal/apptracker/sentry/sentry_tracker.go

+)
+
+type SentryTracker struct {
+	FlushFreq int64


Why do we need this value, it seems it's not being used. Shall we remove it?

that's a good point, where are we using it?

good point, removed

CaioTeixeira95 · 2024-08-27T17:13:09Z

internal/ingest/ingest.go

@@ -25,6 +26,7 @@ type Configs struct {
 	StartLedger          int
 	EndLedger            int
 	LogLevel             logrus.Level
+	AppTracker           sentry.SentryTracker


Here you should use the interface!

Suggested change

AppTracker sentry.SentryTracker

AppTracker apptracker.AppTracker

CaioTeixeira95 · 2024-08-27T17:15:24Z

internal/serve/serve.go

@@ -57,6 +59,9 @@ type Configs struct {
 	HorizonClientURL                   string
 	DistributionAccountSignatureClient signing.SignatureClient
 	ChannelAccountSignatureClient      signing.SignatureClient
+
+	// Error Tracker
+	AppTracker sentry.SentryTracker


Here you should use the apptracker.AppTracker

Error tracking interface and it's Sentry implementation

1265f83

Error tracking interface and it's Sentry implementation Task: https://app.asana.com/0/1207947010297920/1207245825676510

gsoarescosta reviewed Aug 12, 2024

View reviewed changes

cmd/ingest.go Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

gouthamp-stellar added 4 commits August 12, 2024 23:10

go mod tidy

7e1602a

go mod tidy

lint fixes

2c8e467

lint fixes

fixing broken tests

7f328d2

Merge branch 'main' into apptracker

f93903d

CaioTeixeira95 reviewed Aug 16, 2024

View reviewed changes

gouthamp-stellar added 7 commits August 20, 2024 18:56

responding to comments

be1bfe2

Making SentryTracker publicly accessible

8b267f1

missed update

73b60f9

Merge branch 'main' into apptracker

3ab9ec8

Update serve.go

dde7020

go mod tidy

b3495c7

go mod tidy

delete dryrun_tracker

ff1e3df

gouthamp-stellar changed the title ~~Error tracking interface and it's Sentry implementation~~ apptracker: Error tracking interface and it's Sentry implementation Aug 27, 2024

CaioTeixeira95 approved these changes Aug 27, 2024

View reviewed changes

gouthamp-stellar added 2 commits September 10, 2024 16:44

Merge branch 'main' into apptracker

d9a3448

changes based on comments

73a2f0a

gouthamp-stellar merged commit a74bc67 into main Sep 13, 2024
5 checks passed

gouthamp-stellar deleted the apptracker branch September 13, 2024 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apptracker: Error tracking interface and it's Sentry implementation #34

apptracker: Error tracking interface and it's Sentry implementation #34

gouthamp-stellar commented Aug 11, 2024 •

edited

Loading

This comment was marked as outdated.

CaioTeixeira95 left a comment

CaioTeixeira95 Aug 16, 2024

gouthamp-stellar Aug 20, 2024

gouthamp-stellar Aug 21, 2024

CaioTeixeira95 Aug 22, 2024

gouthamp-stellar Aug 27, 2024

daniel-burghardt Sep 12, 2024

daniel-burghardt Sep 12, 2024 •

edited

Loading

gouthamp-stellar Sep 13, 2024

CaioTeixeira95 Aug 16, 2024

gouthamp-stellar Aug 20, 2024

gouthamp-stellar Aug 21, 2024

CaioTeixeira95 Aug 16, 2024

gouthamp-stellar Aug 21, 2024

CaioTeixeira95 Aug 16, 2024

gouthamp-stellar Aug 21, 2024

CaioTeixeira95 Aug 16, 2024

gouthamp-stellar Aug 21, 2024

CaioTeixeira95 left a comment

CaioTeixeira95 Aug 27, 2024

gouthamp-stellar Sep 10, 2024

CaioTeixeira95 Aug 27, 2024

gouthamp-stellar Sep 11, 2024

CaioTeixeira95 Aug 27, 2024

gouthamp-stellar Sep 11, 2024

CaioTeixeira95 Aug 27, 2024

gsoarescosta Aug 27, 2024

gouthamp-stellar Sep 11, 2024

CaioTeixeira95 Aug 27, 2024

gouthamp-stellar Sep 11, 2024

CaioTeixeira95 Aug 27, 2024

gouthamp-stellar Sep 11, 2024

	log.Fatalf("Error initializing App Tracker: %s", err.Error())
	return fmt.Errorf("initializing app tracker: %w", err)

	AppTracker sentry.SentryTracker
	AppTracker apptracker.AppTracker

apptracker: Error tracking interface and it's Sentry implementation #34

apptracker: Error tracking interface and it's Sentry implementation #34

Conversation

gouthamp-stellar commented Aug 11, 2024 • edited Loading

What

Why

Known limitations

Issue that this PR addresses

Checklist

PR Structure

Thoroughness

Release

This comment was marked as outdated.

CaioTeixeira95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-burghardt Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaioTeixeira95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouthamp-stellar commented Aug 11, 2024 •

edited

Loading

daniel-burghardt Sep 12, 2024 •

edited

Loading