Skip to content
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

Update ExportResult: use std::result::Result type #331

Closed
frigus02 opened this issue Nov 2, 2020 · 5 comments · Fixed by #347
Closed

Update ExportResult: use std::result::Result type #331

frigus02 opened this issue Nov 2, 2020 · 5 comments · Fixed by #347
Labels
help wanted Good for taking. Extra help will be provided by maintainers/approvers

Comments

@frigus02
Copy link
Member

frigus02 commented Nov 2, 2020

Update ExportResult to match the latest version in the spec:

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#exportbatch

ExportResult is one of:

  • Success - The batch has been successfully exported. For protocol exporters this typically means that the data is sent over the wire and delivered to the destination server.
  • Failure - exporting failed. The batch must be dropped. For example, this can happen when the batch contains bad data and cannot be serialized.

Since this only defines 2 states now (error and success), we should probably go the idiomatic Rust route and use a std::result::Result.

@frigus02 frigus02 added the good first issue Good for newcomers label Nov 2, 2020
@frigus02 frigus02 mentioned this issue Nov 2, 2020
15 tasks
@jtescher
Copy link
Member

jtescher commented Nov 3, 2020

Is this type actually necessary anymore if there are only two possibilities? or would a result type of some kind be more idiomatic?

@TommyCpp
Copy link
Contributor

TommyCpp commented Nov 3, 2020

I think we can define a ExportError as Failure and use Result as return type.

@frigus02
Copy link
Member Author

frigus02 commented Nov 3, 2020

Yeah, that's a good point. Let's use a Result type. I guess we either wait until #286 and make some kind of ExportError or we start with Box<dyn Error + Send + Sync + 'static>?

I wonder if we should keep the name ExportResult as a type alias or if we just go with a standard Result.

@frigus02 frigus02 added help wanted Good for taking. Extra help will be provided by maintainers/approvers and removed good first issue Good for newcomers labels Nov 3, 2020
@frigus02 frigus02 changed the title Update ExportResult: merge FailedNotRetryable and FailedRetryable into Failure Update ExportResult: use std::result::Result type Nov 3, 2020
@jtescher
Copy link
Member

jtescher commented Nov 3, 2020

Seems like the only purpose here may be calling an error handler as both success and failure will drop the span data. If that is the case then #263 may be the only code path that would use this type?

@frigus02
Copy link
Member Author

frigus02 commented Nov 4, 2020

Yeah, that's how I understand this as well.

In theory someone could implement a custom span processor to control when spans are exported. This would mean they would also be exposed to the export result.

I'd say something like this should be good enough:

type ExportResult = Result<(), Box<dyn std::error::Error + Send + Sync + 'static>>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Good for taking. Extra help will be provided by maintainers/approvers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants