otel instrumentation for outgoing calls using generated clientlibs #19
Conversation
swagger_zipkin/otel_decorator.py
Outdated
| request_options.setdefault('headers', {}) | ||
|
|
||
| # what is the right way to get the Request object. can we use contruct_request | ||
| # https://github.com/Yelp/bravado/blob/master/bravado/client.py#L283C5-L283C22 |
There was a problem hiding this comment.
What is the right way to get the Request object?
There was a problem hiding this comment.
There's no guarantee we're in a pyramid request context when this is being called. We should not rely on it. You'd need whatever the otel equivalent of py_zipkin's Stack type, I suspect. If this was built internally--which I'm somewhat more inclined to--then we could maybe reuse distributed-context's mechanisms? Also, perhaps you're mixing up this library up with pyramid-zipkin, which would have access to the pyramid request context.
There was a problem hiding this comment.
tl;dr
I've used contruct_request from bravado to get the Request object.
Yeah that makes sense this is instrumenting for a outgoing call (not an incoming call where pyramid.request.Request would be available).
For zipkin_decorator it was straightforward since we the zipkin context was used to just inject B3 headers in the outgoing call.
For otel, the context itself is available in the trace object and we can inject zipkin and otel headers. But the additional functionality being added here is the generation of a CLIENT span. For this purpose a bunch of fields are needed: url, path, etc and hence the need for a Request object. This wont be available in the otel/zipkin context.
There was a problem hiding this comment.
Ah, I see I misunderstood which request you meant. I think this should be fine, although I don't think any other bravado decorator has done anything like this before.
acer618
requested review from
benbariteau,
ymilki,
dmfallak,
EOjeah and
kawaiwanyelp
acer618
changed the title
| @contextmanager | ||
| def handle_exception( | ||
| self, | ||
| ) -> Any: |
There was a problem hiding this comment.
The correct return type here is Iterator[None]
| def __dir__(self) -> list[str]: | ||
| return dir(self.resource) # pragma: no cover | ||
|
|
There was a problem hiding this comment.
I don't think overriding __dir__ is desirable here, especially since we're not overriding __getattr__
|
|
||
| def __dir__(self) -> list[str]: | ||
| return dir(self._client) # pragma: no cover |
| try: | ||
| operation(*args, **kwargs) | ||
| except HTTPError as e: | ||
| span.set_attribute("error.type", e.__class__.__name__) | ||
| span.set_status(trace.Status(trace.StatusCode.ERROR, e.message)) | ||
| span.set_attribute("http.response.status_code", e.status_code) | ||
| raise e | ||
|
|
||
| return operation |
There was a problem hiding this comment.
This logic won't work as is. Calling an operation will never raise an HTTPError as you must call result or response on it. You'll need to create a wrapper future class in order to implement the logic you want.
There was a problem hiding this comment.
Thank you for catching this! Will work on a fix.
| try: | ||
| operation(*args, **kwargs) | ||
| except HTTPError as e: | ||
| span.set_attribute("error.type", e.__class__.__name__) | ||
| span.set_status(trace.Status(trace.StatusCode.ERROR, e.message)) | ||
| span.set_attribute("http.response.status_code", e.status_code) | ||
| raise e | ||
|
|
||
| return operation |
swagger_zipkin/otel_decorator.py
Outdated
| span = trace.get_current_span() | ||
| span.set_attribute("error.type", e.__class__.__name__) | ||
| span.set_attribute("http.response.status_code", "500") |
There was a problem hiding this comment.
Is this still correct? Do we even need this try/except anymore?
Proposed change to add otel instrumentation as discussed in # 3 Scope
Prior work done for static clientlibs: https://sourcegraph.yelpcorp.com/python-packages/yelp_openapi/-/blob/yelp_openapi/clientlib/otel_tween.py
Note:
Did not end up creating a new repo: swagger_otel for this purpose. Wanted to re-use decorate_client.py plus zipkin will go away ideally in next 2-3 quarters and we can rename this repo to swagger_otel if necessary.
I've added some comments in the PR where I need help/guidance.