Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add exporter API and more details about SDK implementation #186
Add exporter API and more details about SDK implementation #186
Changes from all commits
160b09e
5c13670
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a
flush
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have
shutdown
the use case for flush is not clear to me.We can add it if we see the clear use case. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - in this PR shutdown and flush are both mentioned. There is no general agreement on whether we need flush or not at this moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep the barrier high for adding new functions. If we don't have a good use case for
flush
let's keep it out for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need more clarification:
Shutdown
is called when the SDK is shutting down, it should block until the clean up is done (or failed).Shutdown
should be called only once for a given exporter.Shutdown
, there should be no further calls toExport
. The exporter should make sure that subsequent calls toExport
return failure immediately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will 2 and 3 to the spec. Good points.
Not sure about 1. For example if
Shutdown
is called for the purpose of restarting the application it be undesirable for block for too long. IfShutdown
is required to block until all data contained in the exporter is delivered to the destination and there is network unavailability this may be too much.I'd like to see some opinions and learn about use cases..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In OpenCensus Python we have a
grace_period
which controls the maximum wait time. Might be something to consider.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can say "Shutdown should not block indefinitely" and let language authors decide if they want to make shutdown timeout to be configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call it
TraceExporter
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to suggestions, I have no strong preference. It seems to me that since it accepts a batch of Spans, SpanExporter may be a better name, but would be great to know other opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this
WrappedError
include information about how SDK should retry?Retry policy could vary among backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, as the text says these are just examples for illustration and language library authors need to think about the details of how exactly the interface and its input/output data types should look like.
If this creates more questions than it answers I would be inclined to completely remove the section. There is no intent here to define an Exporter interface that is idiomatic, complete or even correct at all.