opentelemetry-js
opentelemetry-js copied to clipboard
open-telemetry
Implement `SpanExporter#forceFlush`
Span exporter interface should have a ForceFlush as specified here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#forceflush
Since we have already released the interface stable without this, we can only add it as an optional property on the interface.
Since we have already released the interface stable without this, we can only add it as an optional property on the interface.
Isn't this a semver minor change? I think we discussed a similar issue in API a while ago and that time the agreement was that it is ok that a new minor has new APIs to be implemented by consumers.
@open-telemetry/javascript-maintainers what do you think? @Flarna is right when I talked to ted about this he said it's ok that SDK implementers should have a lower expectation of stability. The spec does state force flush is a required method.
I'm not clearly getting the point here. Is @Flarna suggesting that the new method added in the SDK interfaces can be a non-optional one?
I only read the argument why it needs to be optional and I don't agree with this. New API is semver minor in my opinion.
Looking into the linked spec part it says forceFlush is optional. But that spec is about SDK not exporters. SDK calls SpanProcessor and this one calls SpanExporter. Not sure at all here what forceFlush on SpanExporter should actually do as SpanExporters do not cache/store/collect any data - they just export. So there should be nothing to flush at all.
Maybe abort would make sense to stop a long running export operation but that's a different topic.
Seems I read the wrong part in spec as above link points to TracerProvider#forceFlush.
SpanExporter#forceFlus in turn is optional according to spec and therefore I think it's better to add it as optional.
There might be quite some exporters (e.g. ConsoleExporter,...) where ForceFlush makes no sense so no need for them to implement a dummy.
@Flarna sorry I didn't find an indication in the spec that says that SpanExporter#forceFlush is optional. Would you mind sharing where you find that?
Here spec says The exporter must support two functions: Export and Shutdown.
ForceFlush is a SHOULD API therefore optional.
Thank you for pointing that out. In that sense, I agree that it is a good idea to mark the method as an optional one.
Thank you for the spec pointer, sorry been a little slammed at work, will try and get back to this tomorrow
I'm reading the spec and I see SHOULD requirements for behaviors of ForceFlush but I don't see anywhere that the existence of the function itself is SHOULD. I think the method is required but the behaviors are only SHOULD because some might be impossible (sync shutdown or similar)
@dyladan you think the function should not be optional? The Interface Definition section does not mention forceFlush which I would take to mean it would be optional? I do see the ForceFlush section itself doesn't indicate it should be optional
Spec doesn't make this clear at all. Created https://github.com/open-telemetry/opentelemetry-specification/issues/2652 to get some clarity.
@dyladan I know @sgracias1 mentioned picking this up in the past, but if you're ok with it I'd be happy to pick this issue up since it doesn't look like an update has happened in awhile.
@pichlermarc I noticed @sgracias1 created a PR for this issue here: https://github.com/open-telemetry/opentelemetry-js/pull/3071 but was never merged since a few comments from @dyladan needed to be incorporated. Can I grab those changes and push them up with the appropriate changes made?
Can I grab those changes and push them up with the appropriate changes made?
That's a good question, I'm actually not sure what the procedure for this is yet (I should know it, though).
I reached out to @dyladan as he's usually more knowledgeable in this area, and will let you know as soon as I have an adequate answer. :slightly_smiling_face:
@JacksonWeber, we have done it in the past but try to avoid doing it.
I'd recommend reaching out to @sgracias1 before grabbing and pushing the changes; having a short comment from them on the original PR that it's okay to grab would go a long way. :slightly_smiling_face:
@JacksonWeber, in that case, I think going ahead with the PR would be fine. I'll start a thread on Slack with them (I'll include you in the thread) in case they come back online at some point :slightly_smiling_face:
@pichlermarc @dyladan I created a new PR for this, resolved merge conflicts and tidied up a bit. https://github.com/open-telemetry/opentelemetry-js/pull/3753. Let me know if everything looks good!