Apex log integration(Initial commit) #220

chimanjain · 2021-11-25T12:46:12Z

No description provided.

apexlog/apexlog.go

kyrylo · 2021-11-25T15:38:19Z

apexlog/apexlog.go

+//
+// Valid names are "debug" "info", "warning", "error", and "fatal".  If the name is not
+// recognized, "error" severity is used.
+func setGobrakeSeverity(severity string) {


This name is misleading because it doesn't actually set Gobrake's severity (the library, or the notifier). It just sets state in our handler.

changed the name.

kyrylo · 2021-11-25T15:45:04Z

apexlog/apexlog.go

+
+	msg := fmt.Sprint(args...)
+
+	var theErr error


I have never seen this name being used in Go. Can we use the standard name err instead?

kyrylo · 2021-11-25T15:50:07Z

apexlog/apexlog.go

+	} else {
+		notice = Gobrake.Notice(msg, req, depth)
+	}
+	notice.Context["severity"] = severityName[s]


If s implemented String(), we could call:

Suggested change

notice.Context["severity"] = severityName[s]

notice.Context["severity"] = s.String()

kyrylo · 2021-11-25T15:51:06Z

apexlog/apexlog.go

+	}
+
+	var notice *gobrake.Notice
+	if theErr != nil {


Since you use if else ... you can flip the branches so that we don't negate without a good reason. It's just easier to read if this... then that vs if not this... then that.

apexlog/apexlog.go

kyrylo · 2021-11-25T15:54:08Z

examples/apexlog/main.go

+		}
+	}()
+
+	select {}


This example looks rather complex IMO. What we could do is to:

Set severity to warning

Send a debug log and explain it won't be delivered

Send a fatal log and explain that it will be delivered

Set severity to debug

Send a debug log and explain that it will be delivered

No need for goroutines.

kyrylo · 2021-11-25T15:57:06Z

apexlog/apexlog.go

+func New(notifier *gobrake.Notifier, severity string) *Handler {
+	setGobrakeSeverity(severity)
+	var v Handler
+	Gobrake = notifier


What will happen if two projects use this apexlog integration? Probably only one integration will win and the other one will stop sending logs. Can we make sure we support this scenario?

I am actually not sure why we store notifier on package-level. We store it on the handler itself on the next line, so why not to use it from there?

kyrylo · 2021-11-26T11:32:01Z

examples/apexlog/main.go

+)
+
+func main() {
+	// Initialize the airbrake notifier


This is still present.

kyrylo · 2021-11-26T11:33:31Z

examples/apexlog/main.go

+	// Insert the airbrake notifier instance and set the severity level.
+	// The acceptable severity levels are "debug" "info", "warning", "error", and "fatal".
+	// If the severity level is not recognized, "error" severity is used.
+	airbrakeHandler := apexlog.New(airbrake, "error")


We still pass a string instead of severity.

apexlog/apexlog.go

kyrylo · 2021-11-26T11:38:56Z

apexlog/apexlog.go

+
+// HandleLog method is used for sending notices to airbrake.
+func (h *Handler) HandleLog(e *log.Entry) error {
+	h.notifyAirbrake(0, severity(e.Level), e.Message)


We don't need to have 0 (depth) as a function parameter because it never changes. Let's just remove it and hardcode it to 0 on line 93.

It's just a variable, so that we can build on it in future if we want to or if we want to add some another variable and need some reference.

Could you provide an example of a possible scenario in the future where we want to pass anything other than 0 here?

kyrylo · 2021-11-26T11:40:06Z

examples/apexlog/main.go

+		"user": "tobi",
+	})
+
+	for range time.Tick(time.Millisecond * 500) {


Why do we need a timer?

This example is use used for the apex/log package(the one they used in their package), so we can show that the middleware will work without making any modifications to apex/log and it will be easier for developers to understand who are using apex/log already.

I gave this a run locally and I sent 160 errors to Airbrake within ~15 seconds. I don't think it's a good idea to run this on a loop because our users can leave this running for some time and then they will run out of their error quota or will have to pay more than needed.

I think we need to print these messages without a loop and make the program exit by itself so that we won't waste error quota.

examples/apexlog/main.go

kyrylo · 2021-11-29T09:21:05Z

Is this ready for another review?

chimanjain · 2021-11-29T10:17:03Z

Yes

kyrylo · 2021-11-29T10:19:56Z

Ok, thanks. Please write "PTAL" once you are done with the code. This will help me understand the state of the PR 🙂

kyrylo · 2021-11-29T10:27:11Z

examples/apexlog/main.go

+		"user": "tobi",
+	})
+
+	for range time.Tick(time.Millisecond * 500) {


I gave this a run locally and I sent 160 errors to Airbrake within ~15 seconds. I don't think it's a good idea to run this on a loop because our users can leave this running for some time and then they will run out of their error quota or will have to pay more than needed.

I think we need to print these messages without a loop and make the program exit by itself so that we won't waste error quota.

kyrylo · 2021-11-29T10:28:02Z

examples/apexlog/main.go

+		"user": "tobi",
+	})
+
+	fmt.Println("Check your Airbrake dashboard at https://airbrake.io/projects/:ProjectId to see these log messages")


We should print the real project id being used so that users can actually click on the link

It would be nice to explain that some of the log messages won't be delivered to Airbrake due to severity settings. As of now this message implies all of the messages will be delivered, which isn't actually the case

kyrylo · 2021-11-29T10:38:57Z

apexlog/apexlog.go

+	InfoLog:  "info",
+	WarnLog:  "warn",
+	ErrorLog: "error",
+	FatalLog: "fatal",


When I send ctx.Fatal("oops"), it exits the program but never reports anything to Airbrake.

The apex/log package exits the application internally denying us to log it in airbrake

If we cannot support this, we shouldn't be exposing this severity in our integration because it doesn't do anything useful. In doing so, we should probably document this behavior. If I set up FataLog severity, nothing at all would be sent to Airbrake, which is misleading (I would think that only fatal logs would be sent).

Ok, I will remove that.

chimanjain · 2021-11-29T11:43:24Z

PTAL

kyrylo · 2021-11-29T11:46:23Z

Did you see these comments?

They don't appear to be resolved.

chimanjain · 2021-11-30T11:28:30Z

PTAL

chimanjain requested a review from kyrylo November 25, 2021 12:46

chimanjain closed this Nov 25, 2021

chimanjain deleted the apex-log-integration branch November 25, 2021 12:47

chimanjain restored the apex-log-integration branch November 25, 2021 12:57

chimanjain reopened this Nov 25, 2021

kyrylo suggested changes Nov 25, 2021

View reviewed changes

kyrylo suggested changes Nov 26, 2021

View reviewed changes

kyrylo reviewed Nov 29, 2021

View reviewed changes

kyrylo approved these changes Nov 29, 2021

View reviewed changes

chimanjain requested a review from kyrylo November 30, 2021 11:28

kyrylo approved these changes Nov 30, 2021

View reviewed changes

Apex/log integration with gobrake

a75332d

chimanjain force-pushed the apex-log-integration branch from a531b64 to a75332d Compare November 30, 2021 12:56

chimanjain merged commit ade184b into master Nov 30, 2021

chimanjain deleted the apex-log-integration branch November 30, 2021 12:58

chimanjain restored the apex-log-integration branch December 1, 2021 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apex log integration(Initial commit) #220

Apex log integration(Initial commit) #220

chimanjain commented Nov 25, 2021

kyrylo Nov 25, 2021

chimanjain Nov 25, 2021

kyrylo Nov 25, 2021

chimanjain Nov 25, 2021

kyrylo Nov 25, 2021

kyrylo Nov 25, 2021

kyrylo Nov 25, 2021

kyrylo Nov 25, 2021

kyrylo Nov 26, 2021

kyrylo Nov 26, 2021

kyrylo Nov 26, 2021

chimanjain Nov 26, 2021

kyrylo Nov 26, 2021

kyrylo Nov 26, 2021

chimanjain Nov 26, 2021

kyrylo Nov 29, 2021

kyrylo commented Nov 29, 2021

chimanjain commented Nov 29, 2021

kyrylo commented Nov 29, 2021

kyrylo Nov 29, 2021

kyrylo Nov 29, 2021

kyrylo Nov 29, 2021

chimanjain Nov 29, 2021

kyrylo Nov 29, 2021

chimanjain Nov 29, 2021

chimanjain commented Nov 29, 2021

kyrylo commented Nov 29, 2021

chimanjain commented Nov 30, 2021

	notice.Context["severity"] = severityName[s]
	notice.Context["severity"] = s.String()

Apex log integration(Initial commit) #220

Apex log integration(Initial commit) #220

Conversation

chimanjain commented Nov 25, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrylo commented Nov 29, 2021

chimanjain commented Nov 29, 2021

kyrylo commented Nov 29, 2021

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

chimanjain commented Nov 29, 2021

kyrylo commented Nov 29, 2021

chimanjain commented Nov 30, 2021