-
Notifications
You must be signed in to change notification settings - Fork 790
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
Implement SpanExporter#forceFlush
#3067
Comments
I'll can knock this one out |
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 Maybe |
Seems I read the wrong part in spec as above link points to There might be quite some exporters (e.g. |
@Flarna sorry I didn't find an indication in the spec that says that |
Here spec says |
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 |
@dyladan you think the function should not be optional? The |
Spec doesn't make this clear at all. Created open-telemetry/opentelemetry-specification#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: #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? |
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. 🙂 |
@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. 🙂 |
Should I work on this Issue @pichlermarc @dyladan |
@ashutosh887 I still haven't been able to get ahold of @sgracias1 |
@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 🙂 |
@pichlermarc @dyladan I created a new PR for this, resolved merge conflicts and tidied up a bit. #3753. Let me know if everything looks good! |
Span exporter interface should have a
ForceFlush
as specified here: https:/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#forceflushSince we have already released the interface stable without this, we can only add it as an optional property on the interface.
The text was updated successfully, but these errors were encountered: