Skip to content
This repository was archived by the owner on Dec 23, 2023. It is now read-only.

Exporter/Trace: Refactor to use TimeLimitedHandler.#1890

Merged
songy23 merged 1 commit intocensus-instrumentation:masterfrom
songy23:refactor-trace-exporters
May 10, 2019
Merged

Exporter/Trace: Refactor to use TimeLimitedHandler.#1890
songy23 merged 1 commit intocensus-instrumentation:masterfrom
songy23:refactor-trace-exporters

Conversation

@songy23
Copy link
Copy Markdown
Contributor

@songy23 songy23 commented May 10, 2019

Updates #1880.

The goal of this change is to reduce the code duplication added in recent exporter PRs (e.g #1884 #1885 #1886), as a follow up of #1887.

@dmichel1
Copy link
Copy Markdown
Contributor

hey @songy23 i'm curious about the deadlines you are adding to all the trace exporters. Is this to avoid blocking the SpanExporterImplThread since some of the exporters are running in that thread?

Can the Stackdriver exporter still block the SpanExporterImplThread even though the work is being delegated to the grpc thread pool?

Copy link
Copy Markdown
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented May 10, 2019

Is this to avoid blocking the SpanExporterImplThread since some of the exporters are running in that thread?

Correct, see #1767 (comment).

Can the Stackdriver exporter still block the SpanExporterImplThread even though the work is being delegated to the grpc thread pool?

Stackdriver exporter still blocks the SpanExporterImplThread before deadline is exceeded. After that the exporter just drops any undelivered spans. This is the first step described in #1767 (comment). The second step is to make exporters async (non-blocking).

@dmichel1
Copy link
Copy Markdown
Contributor

dmichel1 commented May 10, 2019

@songy23 makes sense thanks! Glad i'm following along.

@songy23 songy23 merged commit cae2744 into census-instrumentation:master May 10, 2019
@songy23 songy23 deleted the refactor-trace-exporters branch May 10, 2019 22:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants