Add support for OTEL_EXPORTER_JAEGER_TIMEOUT#1863
Add support for OTEL_EXPORTER_JAEGER_TIMEOUT#1863lzchen merged 16 commits intoopen-telemetry:mainfrom
Conversation
srikanthccv
requested review from
a team,
codeboten and
ocelotl
and removed request for
a team
| request, timeout=self._timeout | ||
| ) | ||
| return SpanExportResult.SUCCESS | ||
| except RpcError as error: |
There was a problem hiding this comment.
Were we incorrectly not catching this exception before?
There was a problem hiding this comment.
Yes, We werent' doing anything earlier so this was being handled in processor.
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Thanks for implementing, this is a good cleanup as well. One question regarding the change to the Collector constructor.
|
|
||
| def __init__(self, thrift_url="", auth=None): | ||
| self.thrift_url = thrift_url | ||
| def __init__(self, collector_endpoint, auth=None, timeout_in_millis=None): |
There was a problem hiding this comment.
Was this change intentional? Based on the change description, i wouldnt expect the interface to the Collector class to change.
There was a problem hiding this comment.
I was under the impression that only JaegerExporter shouldn't make changes like this although technically the Collector and AgentClientUDP are not private . I wonder if we should make them private. It's not like Collector is configurable in exporter so I wouldn't expect people use this directly. Thoughts?
There was a problem hiding this comment.
If can make Collector private, it would be a breaking change.
If we change def __init__(self, thrift_url="", auth=None): to def __init__(self, collector_endpoint, auth=None, timeout_in_millis=None): it would be a breaking change as well.
What is making this change breaking is changing the first argument from thrift_url="" to collector_endpoint. It can be easily avoided just by not making this change but I understand, having a thrift-named argument is undesirable, but that is precisely the price to pay for backwards compatibility, right? 🤷
There was a problem hiding this comment.
I agree, but thirft_url="" especially the empty string was annoying so I thought of changing it because Collector is not something end user uses directly. I will revert this back.
ocelotl
left a comment
ocelotl
left a comment
There was a problem hiding this comment.
Leaving a comment regarding the renaming of an argument in __init__.
|
|
||
| def __init__(self, thrift_url="", auth=None): | ||
| self.thrift_url = thrift_url | ||
| def __init__(self, collector_endpoint, auth=None, timeout_in_millis=None): |
There was a problem hiding this comment.
If can make Collector private, it would be a breaking change.
If we change def __init__(self, thrift_url="", auth=None): to def __init__(self, collector_endpoint, auth=None, timeout_in_millis=None): it would be a breaking change as well.
What is making this change breaking is changing the first argument from thrift_url="" to collector_endpoint. It can be easily avoided just by not making this change but I understand, having a thrift-named argument is undesirable, but that is precisely the price to pay for backwards compatibility, right? 🤷
srikanthccv
added
the
Approve Public API check
5 participants
Description
Fixes #1781