Add ExceptionEventData which wraps events that contain exceptions #4162

HaloFour · 2022-02-08T21:57:00Z

Adds ExceptionEventData interface extending EventData to represent an exception recorded to a span. Processors/exporters can access the exception directly to enable alternate formatting.

codecov · 2022-02-08T23:15:00Z

Codecov Report

Merging #4162 (730ed87) into main (b9074f1) will increase coverage by 0.05%.
The diff coverage is 95.00%.

@@             Coverage Diff              @@
##               main    #4162      +/-   ##
============================================
+ Coverage     90.23%   90.28%   +0.05%     
- Complexity     4599     4758     +159     
============================================
  Files           537      558      +21     
  Lines         14082    14618     +536     
  Branches       1347     1402      +55     
============================================
+ Hits          12707    13198     +491     
- Misses          933      960      +27     
- Partials        442      460      +18

Impacted Files	Coverage Δ
...ava/io/opentelemetry/sdk/trace/data/EventData.java	`100.00% <ø> (ø)`
...ace/internal/data/ImmutableExceptionEventData.java	`94.44% <94.44%> (ø)`
.../main/java/io/opentelemetry/sdk/trace/SdkSpan.java	`97.71% <100.00%> (-0.13%)`	⬇️
...ry/sdk/trace/internal/data/ExceptionEventData.java	`100.00% <100.00%> (ø)`
...entelemetry/sdk/metrics/data/LongExemplarData.java	`66.66% <0.00%> (-33.34%)`	⬇️
...telemetry/sdk/metrics/data/DoubleExemplarData.java	`66.66% <0.00%> (-33.34%)`	⬇️
...porter/internal/otlp/traces/SpanLinkMarshaler.java	`95.00% <0.00%> (-5.00%)`	⬇️
...orter/internal/otlp/metrics/ExemplarMarshaler.java	`91.83% <0.00%> (-3.82%)`	⬇️
...emetry/sdk/testing/assertj/ExemplarDataAssert.java	`87.50% <0.00%> (-2.98%)`	⬇️
...etry/exporter/prometheus/PrometheusHttpServer.java	`72.34% <0.00%> (-2.66%)`	⬇️
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9074f1...730ed87. Read the comment docs.

anuraaga · 2022-02-09T07:32:55Z

This seems reasonable to me - we'd probably want to check on the spec repo if anyone else has any thoughts on how to format exceptions across languages

jack-berg · 2022-02-10T22:19:09Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/data/ExceptionEventData.java

+
+import io.opentelemetry.api.common.Attributes;
+
+public interface ExceptionEventData extends EventData {


I believe this can be package private.

Oh nvm. It has to be public so that processors / exporters can check instanceof ExceptionEventData. This seems reasonable to me too.

If we wanted to be cautious we could put this interface in :sdk-extensions:tracing-incubator so users have to add a dependency on an unstable artifact in order to use it.

The SDK would need to know of the interface in order for Span#recordException to add an event that implements it, wouldn't it?

What about putting this in an internal package for now, as we explicitly document anything in internal as being unstable. You would be free to use it, of course, accepting the risk of it changing in the future.

The SDK would need to know of the interface in order for Span#recordException to add an event that implements it, wouldn't it?

Yes, it could place an implementation dependency on :sdk-extensions:tracing-incubator.

What about putting this in an internal package for now, as we explicitly document anything in internal as being unstable.

That would work too, but probably better to not promote using internal classes.

Finished pass at moving the ExceptionEventData interface and implementation to tracing-incubator. Added a helper class that looks for it on the classpath and will use the create static method to create instances, otherwise it defaults to the existing behavior. Not terribly thrilled with the manual use of reflection but it seems that MethodHandles and LambdaMetaFactory aren't available due to the target Android API version and desugaring seems to only apply to them when used with invokedynamic, but hopefully given this code is related to exceptions the overhead of invoking the method is relatively a drop in the bucket.

Also made the implementation of ExceptionEventData defer creating of the common attributes until the getAttributes method is called and the result is memoized, so existing exporters should work as expected if using this implementation. That behavior is certainly up for discussion.

Apologies for the confusion - I was describing for an approach where :sdk:trace took an implementation dependency on :sdk-extensions:tracing-incubator. That way, no reflection would be necessary since ExceptionEventData would always be on the classpath, but users would have to have their own dependency on :sd:tracing-incubator in order to use it.

I'm ok with the internal package approach as well, but not especially fond of the reflection approach.

:sdk-extensions:tracing-incubator already takes a dependency on :sdk:trace, so I don't think that would be an option? If you'd prefer I can definitely move this code into an internal package in :sdk:trace, that would definitely be simpler, but also wouldn't allow anyone to opt-out unless we add some kind of config?

:sdk-extensions:tracing-incubator already takes a dependency on :sdk:trace, so I don't think that would be an option

Ah that's right. The internal package sounds good. Do users need to opt out?

Do users need to opt out?

Not necessarily, just thought it was safer to be able to test it in isolation.

I can get the PR together to move these classes back to :sdk:tracing under an internal package.

jkwatson · 2022-02-16T16:33:37Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/ExceptionEventUtil.java

+
+  private static ExceptionEventFactory initializeEventFactory() {
+    try {
+      Class<?> clazz =


I worry a bit about having this reflection here, which will get logged as a warning on every SDK startup on a JDK 11+. I'm not sure I have a better solution, though.

I saw a similar mechanism is also being used for SDK autoconfigure. Unfortunately I can't rely on the better/safer APIs due to Android compatibility. If there is a better option I am all for it, but I can't think of one that doesn't involve putting some public interface in the SDK class.

...race/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java

anuraaga · 2022-02-18T07:09:43Z

...race/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java

+
+  @Override
+  @Memoized
+  public Attributes getAttributes() {


Suggested change

public Attributes getAttributes() {

public final Attributes getAttributes() {

Memoized methods can't be final in AutoValue.

...race/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java

anuraaga · 2022-02-18T07:11:42Z

...race/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java

+  }
+
+  @Override
+  @Memoized


Just to point out this changes the getAttributes for exceptions from a normal field to volatile field in the new codepath. I think it's OK just want to make sure everyone notices it

Going for the stretch goal of having ExceptionEventData not generate those attributes unless the exporter tries to use them. Assuming the exporter only calls getAttributes once it's probably not actually necessary to memoize them.

If there is hesitation in deferring the generation of those attributes I can definitely back that off. When the interface was in tracing-incubation I felt it was worth exploring. The only attribute I'd prefer to avoid generating is exception.stacktrace as in my Spring codebases they can be quite large, but I don't think it'd a big deal to consider that later.

anuraaga · 2022-02-18T07:13:07Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ExceptionEventData.java

+   *
+   * @return the additional {@link Attributes attributes} of the {@link ExceptionEventData}
+   */
+  Attributes getAdditionalAttributes();


I think this can be a package private method on ImmutableExceptionEventData instead of on this interface

As with that stretch goal I wanted to enable an exporter to access the additional attributes without forcing the semantic attributes for the exception to be created.

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanTest.java

anuraaga

Sorry for delay thought had already hit approve

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ExceptionEventData.java

HaloFour · 2022-03-13T01:56:07Z

Just checking in to see what the thoughts are about this PR.

ghost

Looks good, thanks!

jack-berg

Any reason not to merge this?

HaloFour added 3 commits February 8, 2022 16:54

Add ExceptionEventData which wraps events that contain exceptions

310b239

checkstyle

be6af28

Keep it simple

365791d

HaloFour mentioned this pull request Feb 9, 2022

Add interface for interpreting exceptions recorded to spans #4157

Closed

jack-berg reviewed Feb 10, 2022

View reviewed changes

HaloFour added 2 commits February 14, 2022 15:11

Refactor ExceptionEventData to tracing-incubator

e946244

Clean up reflection code, defer and memoize merged attributes

c24477b

jkwatson reviewed Feb 16, 2022

View reviewed changes

Move ExceptionEventData back to :sdk:trace under internal package

f865e77

HaloFour marked this pull request as ready for review February 18, 2022 01:22

HaloFour requested review from anuraaga, carlosalberto and jsuereth as code owners February 18, 2022 01:22

HaloFour requested a review from a user February 18, 2022 01:22

HaloFour requested a review from Oberon00 as a code owner February 18, 2022 01:22

anuraaga reviewed Feb 18, 2022

View reviewed changes

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanTest.java Outdated Show resolved Hide resolved

Address PR comments

a72f74e

anuraaga approved these changes Feb 28, 2022

View reviewed changes

jkwatson reviewed Feb 28, 2022

View reviewed changes

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ExceptionEventData.java Show resolved Hide resolved

HaloFour added 3 commits February 28, 2022 11:40

Add internal package-info

69c570d

Add internal package verbiage to ExceptionEventData interface

a1d2057

Kick CI

730ed87

jkwatson approved these changes Mar 13, 2022

View reviewed changes

ghost approved these changes Mar 16, 2022

View reviewed changes

jack-berg approved these changes Mar 30, 2022

View reviewed changes

anuraaga merged commit 6bc06f8 into open-telemetry:main Mar 31, 2022

HaloFour deleted the exception-event branch April 4, 2022 18:23

jack-berg mentioned this pull request May 25, 2022

Attach original Exception to Event in Span#recordException #1646

Closed

jamescrosswell mentioned this pull request Aug 3, 2023

Record Exception.Data when calling RecordException open-telemetry/opentelemetry-dotnet#2439

Open

cijothomas mentioned this pull request Oct 10, 2023

Mark exceptions.* conventions stable open-telemetry/semantic-conventions#365

Closed

trask mentioned this pull request Sep 4, 2024

Open io.opentelemetry.instrumentation.logback.appender.v1_0.OpenTelemetryAppender in the library for extension / customisation open-telemetry/opentelemetry-java-instrumentation#12170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ExceptionEventData which wraps events that contain exceptions #4162

Add ExceptionEventData which wraps events that contain exceptions #4162

HaloFour commented Feb 8, 2022 •

edited

Loading

codecov bot commented Feb 8, 2022 •

edited

Loading

anuraaga commented Feb 9, 2022

jack-berg Feb 10, 2022

jack-berg Feb 10, 2022

HaloFour Feb 10, 2022

jkwatson Feb 11, 2022

jack-berg Feb 11, 2022

HaloFour Feb 15, 2022

jack-berg Feb 17, 2022

HaloFour Feb 17, 2022

jack-berg Feb 18, 2022

HaloFour Feb 18, 2022

jkwatson Feb 16, 2022

HaloFour Feb 16, 2022 •

edited

Loading

anuraaga Feb 18, 2022

HaloFour Feb 18, 2022

anuraaga Feb 18, 2022

HaloFour Feb 18, 2022

HaloFour Feb 18, 2022

anuraaga Feb 18, 2022

HaloFour Feb 18, 2022

anuraaga left a comment

HaloFour commented Mar 13, 2022

ghost left a comment

jack-berg left a comment


		import io.opentelemetry.api.common.Attributes;

		public interface ExceptionEventData extends EventData {

	public Attributes getAttributes() {
	public final Attributes getAttributes() {

Add ExceptionEventData which wraps events that contain exceptions #4162

Add ExceptionEventData which wraps events that contain exceptions #4162

Conversation

HaloFour commented Feb 8, 2022 • edited Loading

codecov bot commented Feb 8, 2022 • edited Loading

Codecov Report

anuraaga commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaloFour Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

HaloFour commented Mar 13, 2022

ghost left a comment

Choose a reason for hiding this comment

jack-berg left a comment

Choose a reason for hiding this comment

HaloFour commented Feb 8, 2022 •

edited

Loading

codecov bot commented Feb 8, 2022 •

edited

Loading

HaloFour Feb 16, 2022 •

edited

Loading