Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apmotel span.RecordError should handle nil errors, as per OpenTelemetry's implementation #1565

Closed
jacksehr opened this issue Dec 19, 2023 · 0 comments · Fixed by #1566
Closed
Labels

Comments

@jacksehr
Copy link
Contributor

jacksehr commented Dec 19, 2023

Is your feature request related to a problem? Please describe.
The apmotel bridge has proved extremely helpful in migrating to OpenTelemetry spans/tracers, but there is one noticeable difference inside the Span implementation: RecordError.

The OpenTelemetry SDK implementation of span.RecordError() includes a nil check for the error that is passed in.

In contrast, the apmotel implementation does no such check. While this is understandable, and arguably more idiomatic, it introduces an issue where you cannot seamlessly drop in the bridge and use span.RecordError how you might expect to with the OpenTelemetry SDK.

Describe the solution you'd like
I believe apmotel's span.RecordError should do the same nil check to improve interoperability with the official OpenTelemetry SDK.

Describe alternatives you've considered
With the official SDK, you might do something like:

err := someOperation()
span.recordError(err)

For this code to coexist with the bridge, we'd then have to add a nil check in before the second line, and we'd have to do so everywhere we've already written a recordError usage like it.

While we could do this, and indeed have opted to so far, we've noticed it just leaves us open to missed cases each time we attempt to adopt the bridge in a service to start its migration to OpenTelemetry.

edit: I have attached a draft PR that implements the requested change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant