Improve logging in ledger API server [DPP-231] #8676

kamil-da · 2021-01-28T18:40:38Z

Motivation
Troubleshooting is made unnecessarily hard by lack of adequate logging. Every client request should be logged at the info level as it enters and leaves the system.

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

kamil-da · 2021-01-28T18:43:13Z

@mziolekda Could you review changes and point out which information worth logging I haven't included in this draft?

stefanobaghino-da

Thanks, it basically looks ok, I've added a few notes here and there.

...ation-api/src/main/scala/platform/apiserver/services/admin/ApiPackageManagementService.scala

...tion-api/src/main/scala/platform/apiserver/services/admin/ApiParticipantPruningService.scala

...gration-api/src/main/scala/platform/apiserver/services/admin/ApiPartyManagementService.scala

ledger/participant-integration-api/src/main/scala/platform/store/dao/JdbcLedgerDao.scala

libs-scala/contextualized-logging/BUILD.bazel

stefanobaghino-da

LGTM (but maybe wait for someone from your team to approve as well, if you think that makes sense)

...ation-api/src/main/scala/platform/apiserver/services/transaction/ApiTransactionService.scala

...integration-api/src/main/scala/platform/apiserver/services/ApiCommandCompletionService.scala

...ation-api/src/main/scala/platform/apiserver/services/transaction/ApiTransactionService.scala

ledger/participant-integration-api/src/main/scala/platform/store/dao/JdbcLedgerDao.scala

stefanobaghino-da · 2021-02-05T13:28:59Z

Judging from @mziolekda's most recent comments it appears that many of the keys that are being inserted in this PR were actually already there. It would probably be best to have a second look at this.

tudor-da · 2021-02-10T12:53:35Z

...integration-api/src/main/scala/platform/apiserver/services/ApiCommandCompletionService.scala

+  ): Map[String, String] =
+    Map(
+      logging.commandId(commandId),
+      "statusCode" -> statusCode.map(_.toString).getOrElse(""),


Suggested change

"statusCode" -> statusCode.map(_.toString).getOrElse(""),

"statusCode" -> statusCode.mkString,

Good catch!

Actually it's not possible with our current Scala compiler settings. Specifically, compilation will fail with:

error: [wartremover:Option2Iterable] Implicit conversion from Option to Iterable is disabled - use Option#toList instead

I could use statusCode.toList.mkString but it looks a bit strange to create a list only for mkString. What do you think?

...ration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala

ledger/participant-integration-api/src/main/scala/platform/indexer/ExecuteUpdate.scala

mziolekda

It looks ok now, thanks

…nagement, PackageManagement, ParticipantPruning and PartyManagement CHANGELOG_BEGIN - Logging incoming requests in API services CHANGELOG_END

…tionService CHANGELOG_BEGIN - Logging transactions and transaction trees returned by the ApiTransactionService CHANGELOG_END

oliverse-da · 2021-02-18T09:39:18Z

...cipant-integration-api/src/main/scala/platform/apiserver/services/ApiSubmissionService.scala

@@ -110,7 +110,7 @@ private[apiserver] final class ApiSubmissionService private (

  override def submit(request: SubmitRequest): Future[Unit] =
    withEnrichedLoggingContext(logging.commands(request.commands)) { implicit loggingContext =>
-      logger.debug("Submitting transaction")
+      logger.info("Submitting transaction")


@kamil-da , don't you feel logging every submission at info level might turn out to be a bit verbose?

@oliverse-da Thank you for the comment.
Actually I think it may be more reasonable to lower the level to debug.
This change was explicitly requested by @mziolekda , so his input might be valuable here as well.

kamil-da force-pushed the kamil-da/improve-logging-in-api-server branch from eec2e6f to ccdb108 Compare February 1, 2021 19:59

kamil-da marked this pull request as ready for review February 1, 2021 19:59

kamil-da requested review from gerolf-da and rautenrieth-da as code owners February 1, 2021 19:59

kamil-da requested a review from a user February 1, 2021 19:59

kamil-da requested a review from stefanobaghino-da as a code owner February 1, 2021 19:59

kamil-da requested a review from mziolekda February 1, 2021 20:00

stefanobaghino-da reviewed Feb 1, 2021

View reviewed changes

kamil-da requested a review from stefanobaghino-da February 2, 2021 10:27

stefanobaghino-da approved these changes Feb 2, 2021

View reviewed changes

kamil-da force-pushed the kamil-da/improve-logging-in-api-server branch from 638ca92 to a22c0c6 Compare February 2, 2021 14:11

kamil-da commented Feb 3, 2021

View reviewed changes

...ation-api/src/main/scala/platform/apiserver/services/transaction/ApiTransactionService.scala Outdated Show resolved Hide resolved

mziolekda requested changes Feb 5, 2021

View reviewed changes

kamil-da force-pushed the kamil-da/improve-logging-in-api-server branch from 9f9242a to 4e072ab Compare February 10, 2021 10:59

tudor-da reviewed Feb 10, 2021

View reviewed changes

...ration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala Show resolved Hide resolved

kamil-da commented Feb 10, 2021

View reviewed changes

ledger/participant-integration-api/src/main/scala/platform/indexer/ExecuteUpdate.scala Show resolved Hide resolved

mziolekda approved these changes Feb 12, 2021

View reviewed changes

kamil-da added 10 commits February 15, 2021 09:49

Added logging incoming requests in API services: Submission, ConfigMa…

63cf239

…nagement, PackageManagement, ParticipantPruning and PartyManagement CHANGELOG_BEGIN - Logging incoming requests in API services CHANGELOG_END

Logging transactions and transaction trees streamed by the ApiTransac…

971bea7

…tionService CHANGELOG_BEGIN - Logging transactions and transaction trees returned by the ApiTransactionService CHANGELOG_END

Logging storing the db in JdbcLedgerDao

27b2d19

Changed log severity

4cdaa1d

Factored out logging util to ContextualizedLogger

7671fba

Review improvements

d3f26c2

Removed unused import

d0257c3

Formatted changes

11ce634

Logging completions stream items

196b2b5

Fixed log message

2a806bd

kamil-da added 8 commits February 15, 2021 10:33

Logging complete transactions and transaction trees

cad4a2a

Removed duplicated keys from logging context

87c0225

Formatted changes

83a55ac

Reduced logging for completions and transactions

6dcd0c0

Removed redundant log

da55ad2

Removed update* prefix in Indexer's logging context

04c5c9d

Minor improvement

a7befef

Minor improvement

869ef17

kamil-da force-pushed the kamil-da/improve-logging-in-api-server branch from b91d438 to 869ef17 Compare February 15, 2021 09:34

kamil-da merged commit 926fb59 into main Feb 15, 2021

kamil-da deleted the kamil-da/improve-logging-in-api-server branch February 15, 2021 10:45

oliverse-da reviewed Feb 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve logging in ledger API server [DPP-231] #8676

Improve logging in ledger API server [DPP-231] #8676

kamil-da commented Jan 28, 2021 •

edited by gerolf-da

Loading

kamil-da commented Jan 28, 2021

stefanobaghino-da left a comment

stefanobaghino-da left a comment

stefanobaghino-da commented Feb 5, 2021

tudor-da Feb 10, 2021

kamil-da Feb 10, 2021

kamil-da Feb 10, 2021

mziolekda left a comment

oliverse-da Feb 18, 2021

kamil-da Feb 18, 2021

	"statusCode" -> statusCode.map(_.toString).getOrElse(""),
	"statusCode" -> statusCode.mkString,

Improve logging in ledger API server [DPP-231] #8676

Improve logging in ledger API server [DPP-231] #8676

Conversation

kamil-da commented Jan 28, 2021 • edited by gerolf-da Loading

Pull Request Checklist

kamil-da commented Jan 28, 2021

stefanobaghino-da left a comment

Choose a reason for hiding this comment

stefanobaghino-da left a comment

Choose a reason for hiding this comment

stefanobaghino-da commented Feb 5, 2021

tudor-da Feb 10, 2021

Choose a reason for hiding this comment

kamil-da Feb 10, 2021

Choose a reason for hiding this comment

kamil-da Feb 10, 2021

Choose a reason for hiding this comment

mziolekda left a comment

Choose a reason for hiding this comment

oliverse-da Feb 18, 2021

Choose a reason for hiding this comment

kamil-da Feb 18, 2021

Choose a reason for hiding this comment

kamil-da commented Jan 28, 2021 •

edited by gerolf-da

Loading