KV: support contextual logging in KeyValueConsumption [KVL-1143] #11288

fabiotudone-da · 2021-10-19T13:28:59Z

Description

This is useful in general but it is also a pre-requisite to use the new error infrastructure, as it automatically logs produced errors and retrieves contextual information, such as the correlation ID, from contextual logging.

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
If you mean to change the status of a component, please make sure you keep the Component Status page up to date.

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.

CHANGELOG_BEGIN CHANGELOG_END

ghost · 2021-10-19T14:20:11Z

...e/kvutils/src/main/scala/com/daml/ledger/participant/state/kvutils/KeyValueConsumption.scala

+  )(implicit
+      loggingContext: LoggingContext
  ): List[Update] = {


Suggested change

)(implicit

loggingContext: LoggingContext

): List[Update] = {

)(implicit loggingContext: LoggingContext): List[Update] = {

ghost · 2021-10-19T14:21:21Z

.../src/main/scala/com/daml/ledger/participant/state/kvutils/api/KeyValueParticipantState.scala

+import java.util.concurrent.CompletionStage
+


Please put this back at the top, and configure your IDE to not change the order of these.

ghost · 2021-10-19T14:21:32Z

ledger/indexer-benchmark/src/app/scala/ledger/indexerbenchmark/Main.scala

@@ -5,7 +5,6 @@ package com.daml.ledger.indexerbenchmark

 import java.nio.file.{Files, Paths}
 import java.util.concurrent.atomic.AtomicLong
-


Please restore this empty line.

ghost · 2021-10-19T14:24:02Z

...ain/scala/com/daml/ledger/participant/state/kvutils/api/KeyValueParticipantStateReader.scala

@@ -103,7 +104,7 @@ object KeyValueParticipantStateReader {
      enableSelfServiceErrorCodes: Boolean,
      timeUpdatesProvider: TimeUpdatesProvider = TimeUpdatesProvider.ReasonableDefault,
      failOnUnexpectedEvent: Boolean = true,
-  ): KeyValueParticipantStateReader =
+  )(implicit loggingContext: LoggingContext): KeyValueParticipantStateReader =


I am not convinced it is helpful to provide the logging context on construction. I would have thought it would be more useful per request.

You are right (sadly), 43cf4db

ghost · 2021-10-19T14:24:25Z

.../src/main/scala/com/daml/ledger/participant/state/kvutils/api/KeyValueParticipantState.scala

@@ -41,7 +42,8 @@ class KeyValueParticipantState(
    writer: LedgerWriter,
    metrics: Metrics,
    enableSelfServiceErrorCodes: Boolean,
-) extends ReadService
+)(implicit loggingContext: LoggingContext)


Similarly, here, I don't think it's helpful.

hubert-da

Agree with comments from Samir, other than that LGTM 👍🏻

…eFactory

…ctory

ghost · 2021-10-19T16:14:07Z

...ain/scala/com/daml/ledger/participant/state/kvutils/api/KeyValueParticipantStateReader.scala

@@ -35,7 +36,7 @@ class KeyValueParticipantStateReader private[api] (
        DamlLogEntry,
        ValueSwitch[Status],
        Option[Timestamp],
-    ) => List[Update],
+    ) => LoggingContext => List[Update],


If you pulled out a trait here, you could make the logging context implicit at the method site.

Agreed, going to look into that separately though as I'd like to leverage tomorrow's release.

@sofiafaro-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @sofiafaro-da is in charge of this release. Commit log: ``` 8ff347d Expand type synonyms lazily (#11282) 2fd200f KV: support contextual logging in KeyValueConsumption [KVL-1143] (#11288) f4df1cc converting server errors to proper client errors (#11184) 525e4ce Add GrpcStatus.toProto(JavaStatus) -> ScalaStatus conversion [KVL-1143] (#11292) 9b00a1a Rotate release rotation (#11291) dd09c38 Upgrade rules-nodejs (#11290) 87f1418 [Short] Remove unnecessary traits from ApiSubmissionServiceSpec (#11254) 6b65a72 Merge npm install steps in create-daml-app tests (#11287) a9f6afb kvutils: Rename `VersionedOffset` to `KVOffset`. (#11286) 98cf8d8 KV: introduce v2 error codes behind a CLI switch [KVL-1140] (#11224) 46f6877 Increase time model skew limits (#11273) 8f94cff kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffsetMutator`. [KVL-1154] (#11277) 81fde97 Bazel: Call `_wrap_rule` directly when building the Scala REPL rule. (#11279) edb2b04 Document scaladoc usage in Bazel (#11278) 6255837 Data dependencies support for reexports (#11147) 2fc7490 [Self-service error codes] Adapt error factories [DPP-656] (#11270) b1a6b11 ledger-api-test-tool: Add static time awareness [KVL-1156] (#11266) 243c120 Add an LF typechecking benchmark (#11276) 2db8c13 Test authorization within Exercise choice. (#11246) ec58ed6 Typecheck nested LF type applications more efficiently (#11253) a940016 Add debugRaw corresponding to traceM in Haskell (#11259) a988579 Fix Scala repl targets (#11272) e4808f6 Extract UtilLF module from daml-lf-conversion to its own library (#11263) 38712c0 sandbox-next - Disable participant command deduplication [KVL-1156] (#11264) 82fa229 Add daml-util-ghc lib (#11260) 68a93cf kvutils - Use the ledger configured time for command dedup [KVL-1149] (#11239) c8cede0 Bump startup time in create-daml-app tests (#11261) 4fac87f Bump the schema version for the JSON API (#11252) 5507670 update NOTICES file (#11256) 0063b10 Retry faster during startup (#11255) 04feb40 Improve reset service tests (#11240) b3375fd update compat versions for 1.18.0-snapshot.20211013.8071.0.514e8b50 (#11237) 9ed1eb3 Address security notice on `ansi-regex` (#11243) 387d0e8 Make compatibility migrations `eternal` (#11242) be899b3 Extract rejection_reason.proto from daml_kvutils.proto [KVL-1090] (#11235) a1d94e1 LF: Create special case class for VersionedContractInstance (#11127) b738988 Release 1.18 snapshot (#11234) 3c26852 LF: Use template Id in exercise node of fixed choice. (#11229) 139b6f3 CI: Set `PROJ_DIR` inside the bash lib, not outside. (#11236) ce64cb2 interfaces: Do some TODOs (#11231) 7a88c7d trigger-service: dev-mode-unsafe flag (#11233) 909a1bf [DPP-417][DDP-612] Adapt ApiSubmissionService to support V2 error codes (#11052) ``` Changelog: ``` - [JSON API] Several kinds of gRPC server errors are now reported with associated HTTP statuses; for example, a Daml-LF interpreter error now returns a 400 instead of a 500, and an exercise on an archived contract returns a 409 Conflict instead of a 500. Errors internal to JSON API (e.g. internal assertion failures) are no longer detailed in the HTTP response; their details are only logged. See `issue #11184 <https://github.com/digital-asset/daml/pull/11184>`__. - [Daml Stdlib] Add `debugRaw` as a convenience wrapper around `traceRaw` when used inside a do-block. `debugRaw` compares to `debug` like `traceRaw` compares to `trace` meaning it expects a `Text` instead of calling `show` on an expression. sandbox-next - Disable participant command deduplication kvutils - Command deduplication uses the configured time model (static/wall) and not running always using wall-clock [JSON API] Solving a bug that could cause the JSON API to return correct result if a contract with the same key is observed twice required a schema change. The JSON API data needs to be dropped and the query store needs to reset. If you are migrating from a previous version, either reset your database manually or start the HTTP JSON API with one of the options that regenerate the schema (`create-only`, `create-if-needed-and-start`, `create-and-start`). ``` CHANGELOG_BEGIN CHANGELOG_END

@sofiafaro-da

This PR has been created by a script, which is not very smart and does not have all the context. Please do double-check that the version prefix is correct before merging. @sofiafaro-da is in charge of this release. Commit log: ``` 8ff347d Expand type synonyms lazily (#11282) 2fd200f KV: support contextual logging in KeyValueConsumption [KVL-1143] (#11288) f4df1cc converting server errors to proper client errors (#11184) 525e4ce Add GrpcStatus.toProto(JavaStatus) -> ScalaStatus conversion [KVL-1143] (#11292) 9b00a1a Rotate release rotation (#11291) dd09c38 Upgrade rules-nodejs (#11290) 87f1418 [Short] Remove unnecessary traits from ApiSubmissionServiceSpec (#11254) 6b65a72 Merge npm install steps in create-daml-app tests (#11287) a9f6afb kvutils: Rename `VersionedOffset` to `KVOffset`. (#11286) 98cf8d8 KV: introduce v2 error codes behind a CLI switch [KVL-1140] (#11224) 46f6877 Increase time model skew limits (#11273) 8f94cff kvutils: Use `VersionedOffsetBuilder` where possible, and introduce `VersionedOffsetMutator`. [KVL-1154] (#11277) 81fde97 Bazel: Call `_wrap_rule` directly when building the Scala REPL rule. (#11279) edb2b04 Document scaladoc usage in Bazel (#11278) 6255837 Data dependencies support for reexports (#11147) 2fc7490 [Self-service error codes] Adapt error factories [DPP-656] (#11270) b1a6b11 ledger-api-test-tool: Add static time awareness [KVL-1156] (#11266) 243c120 Add an LF typechecking benchmark (#11276) 2db8c13 Test authorization within Exercise choice. (#11246) ec58ed6 Typecheck nested LF type applications more efficiently (#11253) a940016 Add debugRaw corresponding to traceM in Haskell (#11259) a988579 Fix Scala repl targets (#11272) e4808f6 Extract UtilLF module from daml-lf-conversion to its own library (#11263) 38712c0 sandbox-next - Disable participant command deduplication [KVL-1156] (#11264) 82fa229 Add daml-util-ghc lib (#11260) 68a93cf kvutils - Use the ledger configured time for command dedup [KVL-1149] (#11239) c8cede0 Bump startup time in create-daml-app tests (#11261) 4fac87f Bump the schema version for the JSON API (#11252) 5507670 update NOTICES file (#11256) 0063b10 Retry faster during startup (#11255) 04feb40 Improve reset service tests (#11240) b3375fd update compat versions for 1.18.0-snapshot.20211013.8071.0.514e8b50 (#11237) 9ed1eb3 Address security notice on `ansi-regex` (#11243) 387d0e8 Make compatibility migrations `eternal` (#11242) be899b3 Extract rejection_reason.proto from daml_kvutils.proto [KVL-1090] (#11235) a1d94e1 LF: Create special case class for VersionedContractInstance (#11127) b738988 Release 1.18 snapshot (#11234) 3c26852 LF: Use template Id in exercise node of fixed choice. (#11229) 139b6f3 CI: Set `PROJ_DIR` inside the bash lib, not outside. (#11236) ce64cb2 interfaces: Do some TODOs (#11231) 7a88c7d trigger-service: dev-mode-unsafe flag (#11233) 909a1bf [DPP-417][DDP-612] Adapt ApiSubmissionService to support V2 error codes (#11052) ``` Changelog: ``` - [JSON API] Several kinds of gRPC server errors are now reported with associated HTTP statuses; for example, a Daml-LF interpreter error now returns a 400 instead of a 500, and an exercise on an archived contract returns a 409 Conflict instead of a 500. Errors internal to JSON API (e.g. internal assertion failures) are no longer detailed in the HTTP response; their details are only logged. See `issue #11184 <https://github.com/digital-asset/daml/pull/11184>`__. - [Daml Stdlib] Add `debugRaw` as a convenience wrapper around `traceRaw` when used inside a do-block. `debugRaw` compares to `debug` like `traceRaw` compares to `trace` meaning it expects a `Text` instead of calling `show` on an expression. sandbox-next - Disable participant command deduplication kvutils - Command deduplication uses the configured time model (static/wall) and not running always using wall-clock [JSON API] Solving a bug that could cause the JSON API to return correct result if a contract with the same key is observed twice required a schema change. The JSON API data needs to be dropped and the query store needs to reset. If you are migrating from a previous version, either reset your database manually or start the HTTP JSON API with one of the options that regenerate the schema (`create-only`, `create-if-needed-and-start`, `create-and-start`). ``` CHANGELOG_BEGIN CHANGELOG_END Co-authored-by: Azure Pipelines Daml Build <[email protected]>

fabiotudone-da changed the title ~~Support contextual logging in KeyValueConsumption [KVL-1143]~~ KV: support contextual logging in KeyValueConsumption [KVL-1143] Oct 19, 2021

Support contextual logging in KeyValueConsumption

4987d12

CHANGELOG_BEGIN CHANGELOG_END

fabiotudone-da force-pushed the fabiotudone-da/kv/participant-state/read/add-contextual-logging branch from 0be8b6b to 4987d12 Compare October 19, 2021 13:34

fabiotudone-da marked this pull request as ready for review October 19, 2021 13:37

fabiotudone-da requested review from a team as code owners October 19, 2021 13:37

ghost reviewed Oct 19, 2021

View reviewed changes

Fix formatting and imports

cfa6a7e

hubert-da reviewed Oct 19, 2021

View reviewed changes

Propagate the contextual logging per-readUpdates call

43cf4db

fabiotudone-da requested a review from meiersi-da as a code owner October 19, 2021 15:20

fabiotudone-da added 2 commits October 19, 2021 17:27

No need for at-construction logging context in LogAppendingReadServic…

4b04ad4

…eFactory

No need for logging context in CommitStrategySupport.newReadServiceFa…

6cac08e

…ctory

fabiotudone-da requested review from a user and hubert-da October 19, 2021 15:30

ghost approved these changes Oct 19, 2021

View reviewed changes

Fix JdbcIndexerSpec

0887be6

fabiotudone-da enabled auto-merge (squash) October 19, 2021 16:39

kamil-da approved these changes Oct 19, 2021

View reviewed changes

Fix RecoveringIndexerIntegrationSpec

c3fbfef

fabiotudone-da merged commit 2fd200f into main Oct 19, 2021

fabiotudone-da deleted the fabiotudone-da/kv/participant-state/read/add-contextual-logging branch October 19, 2021 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KV: support contextual logging in KeyValueConsumption [KVL-1143] #11288

KV: support contextual logging in KeyValueConsumption [KVL-1143] #11288

fabiotudone-da commented Oct 19, 2021 •

edited

Loading

ghost Oct 19, 2021

fabiotudone-da Oct 19, 2021

ghost Oct 19, 2021

fabiotudone-da Oct 19, 2021

ghost Oct 19, 2021

fabiotudone-da Oct 19, 2021

ghost Oct 19, 2021

fabiotudone-da Oct 19, 2021

ghost Oct 19, 2021

fabiotudone-da Oct 19, 2021

hubert-da left a comment

ghost Oct 19, 2021

fabiotudone-da Oct 19, 2021

		@@ -5,7 +5,6 @@ package com.daml.ledger.indexerbenchmark

		import java.nio.file.{Files, Paths}
		import java.util.concurrent.atomic.AtomicLong

KV: support contextual logging in KeyValueConsumption [KVL-1143] #11288

KV: support contextual logging in KeyValueConsumption [KVL-1143] #11288

Conversation

fabiotudone-da commented Oct 19, 2021 • edited Loading

Description

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubert-da left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiotudone-da commented Oct 19, 2021 •

edited

Loading