Skip to content

feat: include backtrace first line for better debug info #1674

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

Merged
merged 4 commits into from
Aug 15, 2024

Conversation

xuan-cao-swi
Copy link
Contributor

Propose a solution/improvement for #1669.

Since we are passing the exception class into handle_error, I think it's beneficial to include the last line of the error in the log message for developers to track down the cause.

I believe a full backtrace is too much information to show; however, the end user can always customize their @error_handler to include it.

I didn't add error handling in the encoding function like as_otlp_span because:

  1. After rescuing, we need to provide a replacement value for spans: sds.map { |sd| as_otlp_span(sd) }; otherwise, the encode function will raise another type of error, which I think is redundant information.
  2. If we provide an empty Opentelemetry::Proto::Trace::V1::Span.new like as_otlp_key_value does, it will raise a single error, but the encode will not throw an error and will export incorrect data (e.g., an empty span), which I think is misleading.

kaylareopelle
kaylareopelle previously approved these changes Aug 9, 2024
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

I like this! Thank you!

As an example, here is the output when I reproduce a known error related to the examples/metrics_sdk/metrics_collect_otlp.rb file when no attributes are given to histogram.record:

E, [2024-08-09T15:03:07.514396 #44506] ERROR -- : OpenTelemetry error: unexpected error in OTLP::MetricsExporter#encode - undefined method `map' for nil - /Users/kreopelle/dev/opentelemetry-ruby/exporter/otlp-metrics/lib/opentelemetry/exporter/otlp/metrics_exporter.rb:274:in `histogram_data_point'

@kaylareopelle kaylareopelle dismissed their stale review August 9, 2024 22:10

I missed the test failures. Dismissing the review until they're fixed.

@xuan-cao-swi
Copy link
Contributor Author

xuan-cao-swi commented Aug 12, 2024

Thanks,

If we provide an empty Opentelemetry::Proto::Trace::V1::Span.new like as_otlp_key_value does, it will raise a single error, but the encode will not throw an error and will export incorrect data (e.g., an empty span), which I think is misleading.

I took second thought, and I think it's ok to pass empty Opentelemetry::Proto::Trace::V1::Span or nil, and add check like L126

My second thought will actually make things more complicated. Either way won't break the program because it will be rescued in encode eventually.

@mwear mwear merged commit 0109aa7 into open-telemetry:main Aug 15, 2024
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants