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

OpenTelemetry: add ability to set StatusCode in errorMapper based on A #702

Merged
merged 9 commits into from
Jul 14, 2023

Conversation

IvanFinochenko
Copy link
Contributor

No description provided.

@IvanFinochenko IvanFinochenko requested a review from a team as a code owner May 30, 2023 08:41
@IvanFinochenko
Copy link
Contributor Author

ISSUE #698

@IvanFinochenko
Copy link
Contributor Author

@grouzen, Hi. When I added A in aspects compiler throws error
inferred type arguments [Nothing,Any,Nothing,Nothing,Unit,Nothing] do not conform to method @@'s type parameter bounds [LowerR <: UpperR,UpperR,LowerE,UpperE >: LowerE,LowerA >: Unit,UpperA >: LowerA] _ <- ZIO.unit @@ span("Child") @@ span[Throwable, Unit]("Root")

but If I set types as in PR it works, but it is not convenient. May be you know how it fix ?

@grouzen
Copy link
Contributor

grouzen commented May 30, 2023

@IvanFinochenko Thanks for the contribution!
I'll check the aspect error you mentioned later.

@grouzen
Copy link
Contributor

grouzen commented Jun 1, 2023

I'm letting you know that I'm playing with the different versions of the ErrorMapper interface because we need to re-imagine it, considering these new requirements. We need to make it straightforward for the end user, and the current one looks messy and non-systematic.

@grouzen
Copy link
Contributor

grouzen commented Jun 15, 2023

@IvanFinochenko Hey!
I've come up with the following interface:

final case class StatusMapper[E, A](
  failure: PartialFunction[E, (StatusCode, Option[Throwable])],
  success: PartialFunction[A, (StatusCode, Option[String])]
)

Also, I think we might want to rename the errorMapper parameters to something like statusMapper since we want to map failed and successful cases to status codes.
For the successful part, it is enough to provide just String instead of Throwable.

What do you think?

@IvanFinochenko
Copy link
Contributor Author

@grouzen Hi
Nice interface, I will try to refactor.
But if we have string in successful part we cannot use method span.recordException or we can make own Exception class and pass this String to message ?

@grouzen
Copy link
Contributor

grouzen commented Jun 22, 2023

@grouzen Hi Nice interface, I will try to refactor. But if we have string in successful part we cannot use method span.recordException or we can make own Exception class and pass this String to message ?

In my understanding, we don't need to call recordException for a successful case. This semantics makes sense to me as an application itself doesn't throw exceptions in successful cases, right?
Hope that makes sense.

@grouzen
Copy link
Contributor

grouzen commented Jun 23, 2023

@IvanFinochenko Also, I think it would be better to return a case class instead of a tuple as a result for both (failure and success) cases. We can make a type of the second field abstract:

final case class StatusMapper[E, A](
  failure: PartialFunction[E, StatusMapperResult[Throwable]],
  success: PartialFunction[A, StatusMapperResult[String]]
)

object StatusMapper {
  final case class StatusMapperResult[T](statusCode: StatusCode, error: Option[T] = None)
}

It has two benefits:

  • named fields that are more convenient for the end user
  • error is None by default so that it can be skipped by the end user if not needed in a particular case

@IvanFinochenko
Copy link
Contributor Author

IvanFinochenko commented Jun 23, 2023

@grouzen What do we do with String in Successful part? Set description in span.setStatus(errorStatus, description) ?

@grouzen
Copy link
Contributor

grouzen commented Jun 23, 2023

@grouzen What do we do with String in Successful part? Set description in span.setStatus(errorStatus, description) ?

Yes

@grouzen
Copy link
Contributor

grouzen commented Jul 4, 2023

@IvanFinochenko Hey! How is it going? Do you need any help with it?

@IvanFinochenko
Copy link
Contributor Author

@grouzen I updated PR

@grouzen
Copy link
Contributor

grouzen commented Jul 11, 2023

@IvanFinochenko Please, resolve the conflict. Also, sbt compileExamples needs to be fixed.
Overall, it LGTM and I'm ready to merge it.

Copy link
Contributor

@grouzen grouzen left a comment

Choose a reason for hiding this comment

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

Fix conflicts, please.

# Conflicts:
#	opentelemetry/src/main/scala/zio/telemetry/opentelemetry/tracing/Tracing.scala
@IvanFinochenko
Copy link
Contributor Author

@grouzen I fixed

@IvanFinochenko
Copy link
Contributor Author

@grouzen In build strange error. Can you check please ?

@grouzen
Copy link
Contributor

grouzen commented Jul 14, 2023

Yeah, I noticed this kind of error a couple of days ago it is not relevant to the main code base so we can forcibly merge the PR, don't worry about it.

Copy link
Contributor

@grouzen grouzen left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution!

@grouzen grouzen merged commit 177fa65 into zio:series/2.x Jul 14, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenTelemetry: add ability to set StatusCode in errorMapper based on A
2 participants