recordException should allow additional attributes

[dvoytenko]

Is your feature request related to a problem? Please describe.

The recordException API only accepts the exception and time arguments.

Describe the solution you'd like

The specification appears to require that this API also allows a user to pass additional attributes. The relevant excerpt:

If RecordException is provided, the method MUST accept an optional parameter to provide any additional event attributes (this SHOULD be done in the same way as for the AddEvent method). If attributes with the same name would be generated by the method already, the additional attributes take precedence.

The potential API update would be:

recordException(exception: Exception, attributes: Attributes, time?: TimeInput): void;
recordException(exception: Exception, time?: TimeInput): void;

[dvoytenko]

Could you pls reopen this issue?

[Zirak]

I'm interested in picking this back up. I think the original PR was great, it seems like the only thing missing to be backwards-compatible with type signatures as well was:

   recordException(
     exception: Exception,
-    attributes?: SpanAttributes,
+    attributes?: SpanAttributes | TimeInput,
     time?: TimeInput

In some local testing after applying that change, it's possible to define an interface such as:

interface OtherSpan extends Span {
  recordException(exception: Exception, time?: TimeInput): void;
}

Which, if I understand correctly, looks like the original downstream use.

[dvoytenko]

@Zirak this sounds great!

[smoke]

What would be the way forward to introduce that feature?
The PR #4071 is pretty decent, beside the braking change for DD trace

One easy, but arguable option to not introduce braking changes would be to make the methods optional.

Instead of https://github.com/dvoytenko/opentelemetry-js/blob/96df3fa8c8e980833a549b821aaaeba4cec872f6/api/src/trace/span.ts#L122C1-L141C11
to have

  /**
   * Sets exception as a span event
   * @param exception the exception the only accepted values are string or Error
   * @param [time] the time to set as Span's event time. If not provided,
   *     use the current time.
   */
  recordException?(exception: Exception, time?: TimeInput): void;

  /**
   * Sets exception as a span event
   * @param exception the exception the only accepted values are string or Error
   * @param [attributes] the attributes that will be added to the error event.
   * @param [time] the time to set as Span's event time. If not provided,
   *     use the current time.
   */
  recordException?(
    exception: Exception,
    attributes?: SpanAttributes,
    time?: TimeInput
  ): void;

Another option is to update the DD trace types definitions https://github.com/DataDog/dd-trace-js/blob/0e9fc0afa6fc7692a95496fa579c0c435db277fd/index.d.ts#L1899-L1905
they are eitherway not properly implementing the method and discarding the extra arguments https://github.com/DataDog/dd-trace-js/blob/0e9fc0afa6fc7692a95496fa579c0c435db277fd/packages/dd-trace/src/opentelemetry/span.js#L230-L236

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[brianphillips]

Do we have a path forward on this issue? Our org is working to migrate from New Relic's proprietary APM agent to OTel and New Relic's noticeError(...) function supports these extra attributes when recording an exception. I'm not even aware of a workaround here aside from reimplementing the recordException(...) method to add the event manually since the event created by recordException is not accessible from the Span object itself.

I'm wondering if #4701 could be re-merged as part of the v2 release of the SDK. And actually, given dd-trace now specifies a maximum version of @opentelemetry/api, I think the original PR could be re-opened and merged.