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: add nil and recording checks to span.RecordError #1566

Merged
merged 4 commits into from
Jan 2, 2024
Merged

apmotel: add nil and recording checks to span.RecordError #1566

merged 4 commits into from
Jan 2, 2024

Conversation

jacksehr
Copy link
Contributor

@jacksehr jacksehr commented Dec 19, 2023

Addresses feature request #1565

From it:

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.

This PR implements the mentioned nil/recording checking.

@jacksehr jacksehr marked this pull request as ready for review December 19, 2023 11:24
@jacksehr jacksehr requested a review from a team as a code owner December 19, 2023 11:24
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @jacksehr! Changes look reasonable to me for otel alignment.

dmathieu
dmathieu approved these changes Jan 2, 2024
@dmathieu
Copy link
Member

dmathieu commented Jan 2, 2024

@elasticmachine run elasticsearch-ci/docs

@dmathieu dmathieu merged commit 9adf983 into elastic:main Jan 2, 2024
13 checks passed
@shravanjeevan
Copy link

@dmathieu Would it be possible to get a minor version release with this change? We are waiting on it to progress some things. Thanks 🙏

@jacksehr jacksehr deleted the add-nil-check-to-span-record-error branch January 14, 2024 23:50
@dmathieu
Copy link
Member

I have released 2.4.8.

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 this pull request may close these issues.

apmotel span.RecordError should handle nil errors, as per OpenTelemetry's implementation
4 participants