feat(otelgin): add support for recording panics #5090

asecondo · 2024-02-14T03:44:36Z

Implements as described in #5088

asecondo · 2024-02-14T03:48:33Z

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

@@ -86,7 +86,18 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 			opts = append(opts, oteltrace.WithAttributes(rAttr))
 		}
 		ctx, span := tracer.Start(ctx, spanName, opts...)
-		defer span.End()
+		defer func() {
+			if cfg.RecordPanicConfig.enabled {


note: added this as an opt-in behavior to preserve existing behavior

I'm not sure we need this option. This is new behavior, not a breaking change.

dmathieu · 2024-02-14T08:41:59Z

CHANGELOG.md

@@ -15,6 +15,7 @@ The next release will require at least [Go 1.21].

 - Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108)
 - Support [Go 1.22]. (#5082)
+- Add support to record panics in `go.opentelemetry.io.contrib/instrumentation/github.com/gin-gonic/gin/otelgin` (#5090)


Suggested change

- Add support to record panics in `go.opentelemetry.io.contrib/instrumentation/github.com/gin-gonic/gin/otelgin` (#5090)

- Add support to record panics in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#5090)

dmathieu · 2024-02-14T08:42:56Z

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

@@ -86,7 +86,18 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 			opts = append(opts, oteltrace.WithAttributes(rAttr))
 		}
 		ctx, span := tracer.Start(ctx, spanName, opts...)
-		defer span.End()
+		defer func() {
+			if cfg.RecordPanicConfig.enabled {


I'm not sure we need this option. This is new behavior, not a breaking change.

dmathieu · 2024-02-14T08:44:00Z

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

+			if cfg.RecordPanicConfig.enabled {
+				if r := recover(); r != nil {
+					err := fmt.Errorf("error handling request: %s", r)
+					span.RecordError(err, oteltrace.WithStackTrace(cfg.RecordPanicConfig.stackTrace))


Similarly here, the config is probably superfluous.

dmathieu · 2024-02-14T08:45:33Z

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

+		defer func() {
+			if cfg.RecordPanicConfig.enabled {
+				if r := recover(); r != nil {
+					err := fmt.Errorf("error handling request: %s", r)


What is the added value of creating a new error here over recording the original one?

If we do want to create a new error, we should wrap the original one:

Suggested change

err := fmt.Errorf("error handling request: %s", r)

err := fmt.Errorf("error handling request: %w", r)

i'll update to record the original error

dmathieu · 2024-02-14T08:46:16Z

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

+				if r := recover(); r != nil {
+					err := fmt.Errorf("error handling request: %s", r)
+					span.RecordError(err, oteltrace.WithStackTrace(cfg.RecordPanicConfig.stackTrace))
+					span.SetStatus(codes.Error, "panic recovered")


Suggested change

span.SetStatus(codes.Error, "panic recovered")

span.SetStatus(codes.Error, err.Error())

dmathieu · 2024-02-14T08:47:46Z

instrumentation/github.com/gin-gonic/gin/otelgin/option.go

@@ -88,3 +129,10 @@ func WithSpanNameFormatter(f func(r *http.Request) string) Option {
 		c.SpanNameFormatter = f
 	})
 }
+
+// WithRecordPanicConfig specifies a configuration for panic recording.
+func WithRecordPanicConfig(cfg RecordPanicConfig) Option {


If we do want to keep this config, how about:

Suggested change

func WithRecordPanicConfig(cfg RecordPanicConfig) Option {

func WithRecordPanicConfig(enabled, stacktrace bool) Option {

Then, there's no need to have a config within a config.

codecov · 2024-02-14T08:52:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.1%. Comparing base (a111e3e) to head (bb56d55).

❗ Current head bb56d55 differs from pull request most recent head 0b7fca7. Consider uploading reports for the commit 0b7fca7 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #5090     +/-   ##
=======================================
- Coverage   61.2%   61.1%   -0.2%     
=======================================
  Files        185     185             
  Lines      11207   13786   +2579     
=======================================
+ Hits        6865    8428   +1563     
- Misses      4142    5162   +1020     
+ Partials     200     196      -4

Files	Coverage Δ
...ation/github.com/gin-gonic/gin/otelgin/gintrace.go	`84.2% <100.0%> (+3.9%)`	⬆️
...ntation/github.com/gin-gonic/gin/otelgin/option.go	`100.0% <100.0%> (ø)`

... and 183 files with indirect coverage changes

MrAlias · 2024-02-14T16:34:02Z

instrumentation/github.com/gin-gonic/gin/otelgin/gintrace.go

@@ -86,7 +86,18 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 			opts = append(opts, oteltrace.WithAttributes(rAttr))
 		}
 		ctx, span := tracer.Start(ctx, spanName, opts...)
-		defer span.End()


This behavior is already in the default SDK's End method:

https://github.com/open-telemetry/opentelemetry-go/blob/e8973b75b230246545cdae072a548c83877cba09/sdk/trace/span.go#L403-L420

Why is this being added here? It seems like an SDK concern not an instrumentation one.

that's a really good question. the general pattern/setup i typically deal with have a middleware pattern that look something like this:

panic recovery -pretty simple and attempts to recover any panics, log them, and sets the response code to 500. this is always at the top to catch any and everything that it can.

otelgin (this middleware)

auth and other middlewares

panics show up in logs via the panic recovery middleware described above and also show up in the spans via the mechanism you linked. thinking about this more, the gap i currently have is less that panics get added to this specific span somehow, and more that the spans generated by the otelgin middleware don't have a span status as error and don't set the http.status_code to anything. when an alert fires for an increase in 5XX error codes, i usually first go to Jaeger and type something like http.status_code=500 or error=true. in the cases of panics, those types of queries don't work.

given that, i think this would leave a few options:

don't update this middleware. update the middleware ordering i currently use to be something like panic recover -> otelgin -> panic recover. this would result in the net result i want, but using the same middleware twice doesn't feel great.

update this middleware roughly as it's currently written (still need to apply feedback with not having configs within configs, etc). this would result in the http.status_code field not being guaranteed, but that's not necessarily the responsibility of this middleware to set that. the span status will still be set to error, which is a strong signal.

update this middleware roughly as written, but also give the user the ability to set a status code if a panic is caught. this feels like adding too much responsibility to this middleware.

i'm leaning towards 2, but i'd love to get other opinions as well :)

Looking at the latest semantic conventions, the error.type attribute should be used here to capture the panic information.

It is a known thing that our HTTP instrumentation is quite behind on semantic conventions, but I think this should be our target. Instead of having configuration to enable this, we should add the error.type attribute if a panic occurred always.

This will require this instrumentation to go through the process of upgrading semconv which will require a migration path.

sorry for the late reply - getting back to this now. i think that makes sense. i'm not familiar with these migrations, but i'll happily give it a shot. @MrAlias i may poke you for help with the migrations if i run into issues or have questions. i'll apply these changes, as well as the suggestions from @dmathieu.

actually it looks like #5092 will take care of the migration plan. i might wait until that PR is merged and then make the changes to this package on top of that. how does that sound?

mxpetit · 2024-04-15T14:20:43Z

Hello,
I'm also using otelgin and would very much like this issue to be merged. Can we have a little update on this?
Thanks a lot!

asecondo · 2024-04-17T03:14:03Z

Hello, I'm also using otelgin and would very much like this issue to be merged. Can we have a little update on this? Thanks a lot!

hey @mxpetit - from the discussion in this thread, my current plan is to wait for #5092 to complete, likely make a few changes on top of that, then look to get this merged.

asecondo requested review from hanyuancheung and a team as code owners February 14, 2024 03:44

asecondo commented Feb 14, 2024

View reviewed changes

dmathieu reviewed Feb 14, 2024

View reviewed changes

MrAlias reviewed Feb 14, 2024

View reviewed changes

asecondo added 2 commits March 8, 2024 20:28

feat(otelgin): add support for recording panics

1a0e9d4

chore: remove superfluous config and MR comments

0b7fca7

asecondo force-pushed the 5088_otelgin-catch-record-panics branch from 1213805 to 0b7fca7 Compare March 9, 2024 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(otelgin): add support for recording panics #5090

feat(otelgin): add support for recording panics #5090

asecondo commented Feb 14, 2024

asecondo Feb 14, 2024

dmathieu Feb 14, 2024

dmathieu Feb 14, 2024

dmathieu Feb 14, 2024

dmathieu Feb 14, 2024

dmathieu Feb 14, 2024

asecondo Mar 9, 2024

dmathieu Feb 14, 2024

dmathieu Feb 14, 2024

codecov bot commented Feb 14, 2024 •

edited

MrAlias Feb 14, 2024

asecondo Feb 14, 2024

MrAlias Feb 15, 2024

asecondo Mar 8, 2024

asecondo Mar 9, 2024

mxpetit commented Apr 15, 2024

asecondo commented Apr 17, 2024

	- Add support to record panics in `go.opentelemetry.io.contrib/instrumentation/github.com/gin-gonic/gin/otelgin` (#5090)
	- Add support to record panics in `go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin`. (#5090)

	err := fmt.Errorf("error handling request: %s", r)
	err := fmt.Errorf("error handling request: %w", r)

	span.SetStatus(codes.Error, "panic recovered")
	span.SetStatus(codes.Error, err.Error())

	func WithRecordPanicConfig(cfg RecordPanicConfig) Option {
	func WithRecordPanicConfig(enabled, stacktrace bool) Option {

feat(otelgin): add support for recording panics #5090

Are you sure you want to change the base?

feat(otelgin): add support for recording panics #5090

Conversation

asecondo commented Feb 14, 2024

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

codecov bot commented Feb 14, 2024 • edited

Codecov Report

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

mxpetit commented Apr 15, 2024

asecondo commented Apr 17, 2024

codecov bot commented Feb 14, 2024 •

edited