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

feat: add OpentelemetryTesla.setup/1 #334

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yordis
Copy link
Contributor

@yordis yordis commented Jun 20, 2024

No description provided.

@yordis yordis force-pushed the feat-add-tesla-instrumentation branch from 18abacf to 501c769 Compare June 20, 2024 08:09
@bryannaegele
Copy link
Collaborator

What's the context here? I don't see any docs or tests, and this seems to create a different way to instrument than middleware.

@yordis
Copy link
Contributor Author

yordis commented Jun 21, 2024

I don't see any docs or tests,

My bad, I am still working on it.

and this creates a different way to instrument than middleware.

It is precisely built on top of the :telemetry instead of adding the middleware.

As of today, https://hexdocs.pm/tesla/Tesla.Middleware.Telemetry.html is the way to add telemetry to Tesla instead of adding OTEL-specific middleware.

That will be in line with the rest of the instrumentation.

  • What does that mean for Tesla.Middleware.OpenTelemetry moving forward?

It Depends ™️

Tesla.Middleware.OpenTelemetry would be only needed when you want to :propagator, not for "telemetry." I am still unsure what that means in practice for the time being. But they can both live together for the time being.

@bryannaegele
Copy link
Collaborator

I believe there were a number of issues around where that middleware is defined and what it contained. Those events have been around for many years. I think it might have included an issue with path params not being processed before telemetry fired.

One issue is this creates a single processor which will process requests from all clients using Tesla. I think in the case of Req, this would create multiple traces/spans if the user has Tesla as the backend and some library uses Tesla directly. That could possibly be addressed in docs but something that you'll need to test.

I don't see a world where this replaces the middleware which provides finer-grained control per client implementation.

@yordis
Copy link
Contributor Author

yordis commented Jun 21, 2024

I don't see a world where this replaces the middleware which provides finer-grained control per client implementation.

Yeah ... that is OK.

So what should the resolution be from your perspective about the topic?

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.

2 participants