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

Log failed IO that are in a Kamon span #4394

Merged
merged 12 commits into from
Oct 23, 2023

Conversation

olivergrabinski
Copy link
Contributor

Closes #4393

@olivergrabinski
Copy link
Contributor Author

The non-CE KamonMonitoring is not concerned by this. Should it be? There will be more logs as the migration goes on.

@@ -62,14 +62,15 @@ object KamonMonitoringCats {
component: String,
tags: Map[String, Any] = Map.empty,
takeSamplingDecision: Boolean = true
)(io: IO[A]): IO[A] =
)(io: IO[A]): IO[A] = {
if (enabled)
buildSpan(name, component, tags).bracketCase(_ => io) {
case (span, ExitCase.Completed) => finishSpan(span, takeSamplingDecision)
case (span, ExitCase.Error(cause)) => failSpan(span, cause, takeSamplingDecision)
Copy link
Contributor

Choose a reason for hiding this comment

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

The log may be better here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would still make sense to log the errors even if Kamon is disabled 🤔

Copy link
Contributor

@imsdu imsdu Oct 20, 2023

Choose a reason for hiding this comment

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

Hum fair point, I think it is ok like this, we can have a refactoring later as it is not only about Kamon anymore.
There is a lot of changes at the moment, it is best to wait a bit before renaming more things

But maybe we should only log only the Rejections ?
Other wise we will be flooded if the database is down for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it requires to move Rejection to the kernel if I'm not mistaken (which to me would actually make more sense than sourcing-psql anyway). Cf. latest commits

imsdu
imsdu previously approved these changes Oct 20, 2023
imsdu
imsdu previously approved these changes Oct 23, 2023
@olivergrabinski olivergrabinski dismissed imsdu’s stale review October 23, 2023 15:10

The merge-base changed after approval.

imsdu
imsdu previously approved these changes Oct 23, 2023
@olivergrabinski olivergrabinski dismissed imsdu’s stale review October 23, 2023 16:01

The merge-base changed after approval.

imsdu
imsdu previously approved these changes Oct 23, 2023
@imsdu imsdu merged commit e2af5d4 into BlueBrain:master Oct 23, 2023
5 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.

Log failed IO that are in a Kamon span
2 participants