Zipkin exporter: Add timeout support and implement shutdown #1799
Conversation
srikanthccv
requested review from
codeboten and
hectorhdzg
and removed request for
a team
| @@ -83,6 +85,7 @@ | |||
| from opentelemetry.exporter.zipkin.node_endpoint import IpInput, NodeEndpoint | |||
| from opentelemetry.sdk.environment_variables import ( | |||
| OTEL_EXPORTER_ZIPKIN_ENDPOINT, | |||
| OTEL_EXPORTER_ZIPKIN_TIMEOUT, | |||
There was a problem hiding this comment.
Should this env var be proposed to the specs?
There was a problem hiding this comment.
PR in spec repo open-telemetry/opentelemetry-specification#1636. It has already two approvals and I don't see any objection coming.
There was a problem hiding this comment.
Nice looks like it merged!
| if self.done: | ||
| logger.warning("Exporter already shutdown, ignoring call") | ||
| return | ||
| self.session.close() |
There was a problem hiding this comment.
what does this do to in-flight requests?
There was a problem hiding this comment.
It doesn't affect the in-flight requests. Internally it uses this pool manager https://urllib3.readthedocs.io/en/1.24.3/reference/index.html#urllib3.poolmanager.PoolManager.clear.
There was a problem hiding this comment.
So it won't drop them but it won't wait for them to finish either? I think we should probably wait for them to finish if it can be done easily.
| self.session.headers.update( | ||
| {"Content-Type": self.encoder.content_type()} | ||
| ) | ||
| self.done = False |
There was a problem hiding this comment.
may be rename to _closed? done is a bit ambiguous. At first glance it is not clear whether this refers to a single batch or entire instance.
There was a problem hiding this comment.
Sounds good, will do.
| local_node_port: Depending on context, this could be a listen port or the | ||
| client-side of a socket. | ||
| max_tag_value_length: Max length string attribute values can have. | ||
| timeout: Maximum time the Zipkin exporter will wait for each batch export. |
There was a problem hiding this comment.
Worth calling out the 10s default?
| local_node_port: Depending on context, this could be a listen port or the | ||
| client-side of a socket. | ||
| max_tag_value_length: Max length string attribute values can have. | ||
| timeout: Maximum time the Zipkin exporter will wait for each batch export. |
There was a problem hiding this comment.
same note about mentioning the default timeout
exporter/opentelemetry-exporter-zipkin-json/src/opentelemetry/exporter/zipkin/json/__init__.py
Show resolved
Hide resolved
...elemetry-exporter-zipkin-proto-http/src/opentelemetry/exporter/zipkin/proto/http/__init__.py
Show resolved
Hide resolved
Description
This adds timeout support for zipkin exporter via constructor param and env var
OTEL_EXPORTER_ZIPKIN_TIMEOUTand implements shutdown. Additionally changed the exporter to use requests.Session to avoid making new connection to zipkin collector on each export call.Part of #346 and #1791