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

Inspect Elixir exception module name in record_exception #804

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

GregMefford
Copy link
Contributor

The main change in this PR is to change the way we report the exception.type attribute when OpenTelemetry.Span.record_exception is called. Currently, it is using to_string, which results in the struct name having an Elixir. prefix on it, which is technically correct, but not what an Elixir developer would expect, since Elixir hides that most of the time. By using inspect instead, we get that functionality here as well.

The bulk of the PR, though, is adding some testing infrastructure that allows me to be able to assert that we're doing the right thing there, since this change is in the opentelemetry_api package, which does not depend on the opentelemetry SDK, where all the span-processing happens, leaving us with only otel_tracer_noop, which just ignores the data we want to assert on. I tried to make it so that it'll be compatible with Erlang tests as well in the future, but I'm not really familiar with how to organize the code there. In Elixir, there's a convention to put this kind of module that's only used in tests in the test/support directory and configure the compiler paths (which I've also done in this PR even though I didn't end up using it because it doesn't "just work" for Erlang files) when we're running in test mode so that it can find them. That way, they're not included in the distributed Hex package because they're not in the main library code. I wasn't sure how to do the same thing for Erlang.

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 65.51724% with 10 lines in your changes missing coverage. Please review.

Project coverage is 20.18%. Comparing base (0a969d1) to head (a56e134).

Files with missing lines Patch % Lines
apps/opentelemetry_api/src/otel_span_mailbox.erl 50.00% 8 Missing ⚠️
apps/opentelemetry_api/src/otel_tracer_test.erl 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #804      +/-   ##
==========================================
+ Coverage   17.32%   20.18%   +2.86%     
==========================================
  Files          24       26       +2     
  Lines         710      738      +28     
==========================================
+ Hits          123      149      +26     
- Misses        587      589       +2     
Flag Coverage Δ
api 20.18% <65.51%> (+2.86%) ⬆️
elixir 20.18% <65.51%> (+2.86%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GregMefford GregMefford changed the title Inspect Elixir exception module name in report_exception Inspect Elixir exception module name in record_exception Nov 27, 2024
@GregMefford
Copy link
Contributor Author

Heh, I missed that there were already some higher-level tests at the top level of the git repo until I saw it failing in CI. 🤦
Oh well, now we also have some infrastructure in place that we can use at the lower level. 😅

@ferd
Copy link
Member

ferd commented Dec 2, 2024

Does this risk changing what you'd see for an Erlang-provided exception when using the Elixir macros compared to what happened prior to this fix?

@GregMefford
Copy link
Contributor Author

GregMefford commented Dec 2, 2024

Does this risk changing what you'd see for an Erlang-provided exception when using the Elixir macros compared to what happened prior to this fix?

It's technically a change regardless since it's intentionally formatting the exception differently, but I'd argue it's fixing a bug compared to what would be expected on the Elixir side. But the only change in this PR is when you call the Elixir version of the API (OpenTelemetry.Span.record_exception) rather than the Erlang one (otel_span:record_exception), and pass in an Elixir Exception struct (it's pattern-matching only the ones that have that shape and silently ignoring others).

The Erlang version of the API does something different in terms of formatting, and I'd argue that maybe we should figure out how to converge them, given that libraries could end up calling one or the other depending on how they integrate, and IMO it's really more about how you want to format exceptions / stacktraces, perhaps based on a configuration option, even. Maybe if your app is mostly in Erlang, but has some Elixir code, you'd prefer to see your Elixir exceptions still formatted as Erlang, for example.

@ferd
Copy link
Member

ferd commented Dec 2, 2024

Yeah to be clear my concern here was whether dropping Elixir from the elixir exception would also change the non-Elixir exception handled at the same call-site but it comes from an Erlang lib.

@bryannaegele
Copy link
Contributor

Don't know if you saw this @GregMefford but I have an implementation in Erlang in the cowboy instrumentation.

https://github.com/open-telemetry/opentelemetry-erlang-contrib/blob/main/instrumentation/opentelemetry_cowboy/src/opentelemetry_cowboy.erl#L403-L417

@GregMefford
Copy link
Contributor Author

I have an implementation in Erlang in the cowboy instrumentation.

Nice, I did see that my Cowboy error spans had a different error formatting in prod, but hadn't dug into how it's working yet. That'll be a good starting point for converging on a standard format.

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

Successfully merging this pull request may close these issues.

3 participants