feat(otelgin): enhance gin error tracking with span recording

[flc1125]

No description provided.

[dmathieu]

Could you test this change?
By checking there's indeed an error here for example: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go#L123

Also, I wonder if we need to keep the gin.errors attribute. While removing it is a breaking change, keeping it is redundant.

[dmathieu]

Side note that otelgin has no owner, so it may be removed soon.
#6190

[flc1125]

Side note that otelgin has no owner, so it may be removed soon. #6190

Okay, will this package still accept new PRs at present?

[dmathieu]

It does.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.9%. Comparing base (9bbebd9) to head (59de17e).
Report is 1 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #6346   +/-   ##
=====================================
  Coverage   66.9%   66.9%           
=====================================
  Files        193     193           
  Lines      15652   15654    +2     
=====================================
+ Hits       10480   10482    +2     
  Misses      4881    4881           
  Partials     291     291           

Files with missing lines	Coverage Δ
...ation/github.com/gin-gonic/gin/otelgin/gintrace.go	84.3% <100.0%> (+0.3%)	⬆️

[flc1125]

#6190

I think I can give it a try.

If I come to maintain it, what do I need to do?

[dmathieu]

That's documented in the issue, as well as in https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/CONTRIBUTING.md#code-owners.

[flc1125]

Could you test this change? By checking there's indeed an error here for example: main/instrumentation/github.com/gin-gonic/gin/otelgin/test/gintrace_test.go#L123

Also, I wonder if we need to keep the gin.errors attribute. While removing it is a breaking change, keeping it is redundant.

It's done.

[flc1125]

Side note that otelgin has no owner, so it may be removed soon.

I spent some time reading through the requirements. I understand that I may still need to make some contributions before applying. If so, I need some time.

[flc1125]

link: #5252

Changes made

[dmathieu]

This is starting to look good 😸

[pellared]

Just nit comments. Can you please address them before we merge?

[dmathieu]

Could you resolve the conflicts?

[flc1125]

Could you resolve the conflicts?

It's done.

[akats7]

@dmathieu I could pick up ownership of this module so that it isn't removed, should I just open a PR on the codeowners file?

[dmathieu]

Yes, that should be enough (link the otelgin deprecation issue in that PR).

[pellared]

@dmathieu I could pick up ownership of this module so that it isn't removed, should I just open a PR on the codeowners file?

Could you review this PR if you want to be an code owner? 😉

[MrAlias]

@dmathieu I could pick up ownership of this module so that it isn't removed, should I just open a PR on the codeowners file?

Could you review this PR if you want to be an code owner? 😉

cc @akats7 (I think)

[akats7]

lgtm, my only real thought is I personally always find it convenient to have the error message at least partially included directly on the span instead of only having the span events. Since span events are sometimes (always?) stored in separate table it could becomes an additional step to view the message depending on the platform you use.

For example I know a lot of the java instrumentation just adds error: true as an attribute and then creates a span event, and we've had teams complain that the error message has no contextual info

[flc1125]

lgtm, my only real thought is I personally always find it convenient to have the error message at least partially included directly on the span instead of only having the span events. Since span events are sometimes (always?) stored in separate table it could becomes an additional step to view the message depending on the platform you use.

For example I know a lot of the java instrumentation just adds error: true as an attribute and then creates a span event, and we've had teams complain that the error message has no contextual info

This is indeed a problem, and it usually depends on what backend tools we are using.

Fortunately, when the entire span contains some abnormal events, the tool I am currently using allows me to see the exception information directly in the span interface. Of course, for more detailed information, I still need to switch to view it in Events.

——

Back to this PR, due to the OTEL standard specifications, I suggest we still maintain storing exceptions in Events. After all, this project is for the majority of users, and we need to present ourselves as professionally as possible and in accordance with the standards. 😁

[akats7]

Yes we need to record the errors as a span event, what I'm getting at is when it comes to:

	span.SetAttributes(attribute.String("gin.errors", c.Errors.String()))

or

	span.SetStatus(codes.Error, "gin error")

I do find including the error string to have value