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

🐛 Fix OpentelemetryFinch handler on Finch.stream_while #398

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rparcus
Copy link

@rparcus rparcus commented Oct 18, 2024

Fixes #327

Copy link

linux-foundation-easycla bot commented Oct 18, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@rparcus rparcus changed the title 🐛 Fix handler on streams 🐛 Fix handler on Finch streams Oct 18, 2024
@rparcus rparcus changed the title 🐛 Fix handler on Finch streams 🐛 Fix OpentelemetryFinch handler on Finch.stream_while Oct 18, 2024
@rparcus
Copy link
Author

rparcus commented Nov 20, 2024

Hello 👋
Without this fix it is impossible to instrument anything that uses streaming + finch.
Is there anything I can do to further help with the review process?

@@ -44,8 +44,17 @@ defmodule OpentelemetryFinch do

status =
case meta.result do
{:ok, response} -> response.status
_ -> 0
{:ok, response} when is_map(response) ->
Copy link
Author

Choose a reason for hiding this comment

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

The main issue was here. Not every response contains a status key as it depends on the implementation of the streaming callback functions.

A fallback case is also added. In this fix, I'm not trying to come up with a general solution but just one that works for the patterns I saw. The tests use callbacks that return values in the patterns I was already working with.

Copy link

@lud-wj lud-wj Dec 2, 2024

Choose a reason for hiding this comment

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

I guess you could just match on the presence of the status:

case meta.result do
  {:ok, %{status: status}} when is_integer(status) -> status
  {:ok, {_request, %{status: status}}} when is_integer(status) -> status
  {:ok, {status, _headers, _body}} when is_integer(status) -> status
  _ -> 0
end

Because there is a risk that even when response is a map, response.status may not be set. As far as I can tell that part of the event is the stream accumulator and it could be anything, any term.

Copy link

@lud-wj lud-wj Dec 3, 2024

Choose a reason for hiding this comment

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

(edit fixed my clauses 😅 )

@lud-wj
Copy link

lud-wj commented Dec 2, 2024

+1 for this PR.

We are using the Langchain lib with streams, and it is not compatible with OTEL.

@rparcus
Copy link
Author

rparcus commented Dec 3, 2024

Thanks for the suggestion above. I think it's great @lud-wj.
Done in 9ac5b88

+1 for this PR.

We are using the Langchain lib with streams, and it is not compatible with OTEL.

That is exactly why this PR exists ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opentelemetry_finch handler is failing and detaching
3 participants