Span.recordException must allow additional attributes according to spec

[vreynolds]

Not sure if this ought to be labelled a "bug" or "discussion". I noticed a discrepancy between span.recordException and the spec

Specifically, this part (current implementation does not allow passing additional attributes):

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.

• [x] This only affects the JavaScript OpenTelemetry library
• [ ] This may affect other libraries, but I would like to get opinions here first

[vmarchaud]

I'll move this into the API repo (https://github.com/open-telemetry/opentelemetry-js-api/blob/main/src/trace/span.ts#L128)

[vreynolds]

@dyladan regarding API being 1.0.0, can you help me understand the compatibility requirements? I have an implementation that updates the interface from

  recordException(
    exception: Exception,
    time?: TimeInput
  )

to

  recordException(
    exception: Exception,
    attributesOrTime?: SpanAttributes | TimeInput,
    time?: TimeInput
  )

This will not break current API [1.0.0] + new SDK (with new implementation), but will break new API [1.x.y] (with new signature) + current SDK [0.22.0], because if someone tries to just pass the attributes to the current SDK, it will attempt to parse them as TimeInput and fail. Is this a no-go situation?

Alternatively, I could try changing the interface by adding a third optional arg, which should not break either combination of API/SDK. It's more awkward to consume (since users would need to pass an undefined placeholder for time), and the spec calls out that the signature should match addEvent.

Another option you mentioned last week is to accept and document the discrepancy.

[dyladan]

In a minor version bump, it is acceptable to introduce changes which break interface compatibility with the SDK, requiring the SDK to implement the change in the next version. It is not acceptable to make a change which breaks API users. Your change is acceptable because anyone calling recordException will still continue to work. If those users want to upgrade the API they are using, they will need to ensure they use an SDK which is compatible with the newer API version.

bump	can break API callers (end users)	can break API implementers (sdk compatibility)
major	yes	yes
minor	no	yes
patch	no	no

[vreynolds]

Leaving this on the back burner per https://github.com/open-telemetry/opentelemetry-js-api/pull/97#issuecomment-868485468

[longility]

Not sure who could help with this, but I'll ping a few. Sorry, if I'm creating noise for you.

@Flarna @MSNev @vmarchaud @obecny @blumamir,

Is it possible to prioritize this into v1.1 release?

I have a couple of use cases where I'm doing workarounds, and it would be great if this can get in. I think the effort is minimal and there are PRs that have been related to this before such as #97 and https://github.com/open-telemetry/opentelemetry-js/pull/2298 To where it could be as simple as re-open, spot check and possibly final tweaks.

I have minimally talked with @dyladan and he suggested I bring it up to see what y'all think.

[Flarna]

I think the https://github.com/open-telemetry/opentelemetry-js-api/pull/97 was on a good path. My main concern were the non backward compatible changes there which should be fixable.

Personally I have no special opinion if this is added in 1.1, 1.2 or later. I think it's mostly a matter of finding someone to do the actual work.

[dyladan]

Personally I think if we want to include this feature we should do it in such a way that at least it doesn't break existing SDK, even if the feature doesn't work. For example if it is added as a new optional parameter at the end of the method then the existing SDK will just ignore it but at least won't cause compile incompatibilities or crashes.

[dyladan]

Personally I think it would be good to include this if we can get the work done in time. @longility would you volunteer to add this (or @vreynolds)? It would need to be added to both the API and the SDK.

[longility]

Personally I think if we want to include this feature we should do it in such a way that at least it doesn't break existing SDK, even if the feature doesn't work. For example if it is added as a new optional parameter at the end of the method then the existing SDK will just ignore it but at least won't cause compile incompatibilities or crashes.

@dyladan I tested real quick and this contract recordException(exception: Exception, attributesOrTime?: SpanAttributes | TimeInput, time?: TimeInput): void; that is almost consistent with existing addEvent(name: string, attributesOrStartTime?: SpanAttributes | TimeInput, startTime?: TimeInput): this; will not cause compile error in sdk.

Should we go ahead an align to this contract, or do we need more input?

[dyladan]

There are SDKs "in the wild" that depend on API ^1.0.x (example) which still receive significant downloads. These SDKs could break if someone calls recordException(exception, attributes, time) when they are expecting recordException(exception, time) (they will attempt to treat the attributes as time). I would prefer to have a true backwards compatible contract like recordException(exception: Exception, time?: TimeInput, attributes?: Attributes): void; or recordException(exception: Exception, time?: TimeInput, options: RecordExceptionOptions): void;

[longility]

For simplicity and until we want to making clean breaking changes to where the ideal looks like this recordException(exception: Exception, options: RecordExceptionOptions): void;, I'm going to keep it simple and do recordException(exception: Exception, time?: TimeInput, attributes?: Attributes): void;

[dyladan]

That works for me. What do the other @open-telemetry/javascript-maintainers think? It is unfortunate we can't match the addEvent method, but I guess it's just a compromise we have to make.

[dyladan]

I don't want to commit to adding this for 1.1 before I get the other maintainers input

[longility]

Yep, and I don't want to commit to work, until we get alignment on contract.

It is unfortunate we can't match the addEvent method, but I guess it's just a compromise we have to make.

I think maybe at some point in future, we can decide to deprecate addEvent and recordException methods for overloading methods that take in options as part of a bigger breaking change version.

[dyladan]

Yep, and I don't want to commit to work, until we get alignment on contract.

@open-telemetry/javascript-maintainers please respond or react with 👍 / 👎 on adding an optional third parameter to recordException

[vreynolds]

@longility sounds like you have bandwidth and motivation to get this over the line 😄 If not, I can pick this back up next week.

[longility]

@vreynolds heh, I'm waiting for the thumbs up and depending on when, I may or may not be available again. We will just coordinate.

[robross0606]

Is there any workaround for this until it is fixed to be more like addEvent()?

[dyladan]

API is moved back into the main repo. Closing in favor of https://github.com/open-telemetry/opentelemetry-js/issues/3309