-
Notifications
You must be signed in to change notification settings - Fork 310
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
General OpenTelemetry backend #1411
Conversation
.../open-telemetry-backend/src/main/scala/sttp/client3/opentelemetry/OpenTelemetryBackend.scala
Outdated
Show resolved
Hide resolved
.../open-telemetry-backend/src/main/scala/sttp/client3/opentelemetry/OpenTelemetryBackend.scala
Outdated
Show resolved
Hide resolved
private implicit val _monad: MonadError[F] = responseMonad | ||
type PE = P with Effect[F] | ||
|
||
def send[T, R >: PE](request: Request[T, R]): F[Response[T]] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the impl looks quite different from the zio one, but I suspect that's beacuse the zio library was doing some of the work that happens here? I'm not familiar with other opentelemetry integrations, if that's what is usually done, fine :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used example from io.opentelemetry.api.trace.Tracer
class with automatic context propagation it seemed reasonable
.../open-telemetry-backend/src/main/scala/sttp/client3/opentelemetry/OpenTelemetryBackend.scala
Outdated
Show resolved
Hide resolved
.../open-telemetry-backend/src/main/scala/sttp/client3/opentelemetry/OpenTelemetryBackend.scala
Outdated
Show resolved
Hide resolved
.../open-telemetry-backend/src/main/scala/sttp/client3/opentelemetry/OpenTelemetryBackend.scala
Outdated
Show resolved
Hide resolved
.../open-telemetry-backend/src/main/scala/sttp/client3/opentelemetry/OpenTelemetryBackend.scala
Outdated
Show resolved
Hide resolved
.../open-telemetry-backend/src/main/scala/sttp/client3/opentelemetry/OpenTelemetryBackend.scala
Outdated
Show resolved
Hide resolved
|
||
private def prepareBaseAttributes[R >: PE, T](request: Request[T, R]) = { | ||
Attributes.of( | ||
AttributeKey.stringKey("http.method"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't these keys available via SemanticAttributes
?
Also, why do we set the method & url, but not the target, scheme, host, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are sure that SemanticAttributes
are reachable from this project ?
I decide to set same params as for opentracingbackend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I saw the constant in the docs, maybe it's not in the api
though the example in the docs for a http client call sets only method & url, so maybe that's standard
.../open-telemetry-backend/src/main/scala/sttp/client3/opentelemetry/OpenTelemetryBackend.scala
Outdated
Show resolved
Hide resolved
Reading through https://opentelemetry.io/docs/instrumentation/java/manual/, I see that there's also a metrics api. Maybe we should have both an |
Why not I will make it in separate PR. |
@Pask423 looks good. Fine with a separate PR and removing open tracing :) |
closes #1402