Move logging value definitions alongside their objects. #10439

ghost · 2021-07-28T15:58:26Z

Rather than disassembling them elsewhere, let's do it in one place.

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.

There's no reason for it to need `equals`, etc. CHANGELOG_BEGIN CHANGELOG_END

Instead of having a function, let's use `ToLoggingContext`. This also adds a couple of missing items, and always logs `workflowId`.

Instead of having a function, let's use `ToLoggingContext`. This changes some of the logging context structure, but otherwise everything remains the same.

kamil-da

Looks good!
I've left a single comment for a possible follow-up PR.

kamil-da · 2021-07-28T19:03:43Z

ledger/participant-state/src/main/scala/com/daml/ledger/participant/state/v2/Update.scala

+  private object Logging {
+    def recordTime(timestamp: Timestamp): LoggingEntry =
+      "recordTime" -> timestamp.toInstant
+
+    def submissionId(id: Ref.SubmissionId): LoggingEntry =
+      "submissionId" -> id
+
+    def submissionIdOpt(id: Option[Ref.SubmissionId]): LoggingEntry =
+      "submissionId" -> id
+
+    def participantId(id: Ref.ParticipantId): LoggingEntry =
+      "participantId" -> id


I understand that we would like to keep some of T => LoggingEntry methods private to specific classes/packages because of required domain context that is known only in such class/package. However, we could at least consider moving methods for Ref types to a common object. Currently we have multiple definitions for several of Ref types, e.g. for Ref.SubmissionId.
What do you think?

I’d be fine with moving them to Ref but doesn’t have to be in this PR.

I'd be somewhat OK with this.

TBH I'm not sure these methods add too much but clearly others find value in them, so I guess there's something to it. I was tempted to inline them all.

That said, I'd prefer to find a way to derive these automatically from the type. Right now SubmissionId and some others are just type aliases to LedgerString though, so that wouldn't work.

cocreature

Thanks!

cocreature · 2021-07-29T06:43:56Z

ledger/participant-state/src/main/scala/com/daml/ledger/participant/state/v2/Update.scala

+  private object Logging {
+    def recordTime(timestamp: Timestamp): LoggingEntry =
+      "recordTime" -> timestamp.toInstant
+
+    def submissionId(id: Ref.SubmissionId): LoggingEntry =
+      "submissionId" -> id
+
+    def submissionIdOpt(id: Option[Ref.SubmissionId]): LoggingEntry =
+      "submissionId" -> id
+
+    def participantId(id: Ref.ParticipantId): LoggingEntry =
+      "participantId" -> id


I’d be fine with moving them to Ref but doesn’t have to be in this PR.

fabiotudone-da · 2021-07-29T06:53:07Z

ledger/ledger-api-domain/src/main/scala/com/digitalasset/ledger/api/domain.scala

+      )
+  }
+
+  /** @param party      The stable unique identifier of a Daml party.


Suggested change

/** @param party The stable unique identifier of a Daml party.

/** @param party The stable unique identifier of a Daml party.

fabiotudone-da · 2021-07-29T06:54:59Z

...r/participant-state/src/main/scala/com/daml/ledger/participant/state/v2/CompletionInfo.scala

+  * @param actAs                   the non-empty set of parties that submitted the change.
  * @param applicationId          an identifier for the Daml application that submitted the command.
  * @param commandId              a submitter-provided identifier to identify an intended ledger change
  *                               within all the submissions by the same parties and application.
  * @param optDeduplicationPeriod The deduplication period that the [[WriteService]] actually uses for the command submission.
-  *                               It may differ from the suggested deduplication period given to [[WriteService.submitTransaction]] or [[WriteService.rejectSubmission]].
-  *                               For example, the suggested deduplication period may have been converted into a different kind or extended.
-  *                               The particular choice depends on the particular implementation.
-  *                               This allows auditing the deduplication guarantee described in the [[ReadService.stateUpdates]].
+  *                                It may differ from the suggested deduplication period given to [[WriteService.submitTransaction]] or [[WriteService.rejectSubmission]].
+  *                                For example, the suggested deduplication period may have been converted into a different kind or extended.
+  *                                The particular choice depends on the particular implementation.
+  *                                This allows auditing the deduplication guarantee described in the [[ReadService.stateUpdates]].
  *
-  *                               Optional as some implementations may not be able to provide this deduplication information.
-  *                               If an implementation does not provide this deduplication information,
-  *                               it MUST adhere to the deduplication guarantee under a sensible interpretation
-  *                               of the corresponding [[SubmitterInfo.deduplicationPeriod]].
-  * @param submissionId An identifier for the submission that allows an application
-  *   to correlate completions to its submissions.
+  *                                Optional as some implementations may not be able to provide this deduplication information.
+  *                                If an implementation does not provide this deduplication information,
+  *                                it MUST adhere to the deduplication guarantee under a sensible interpretation
+  *                                of the corresponding [[CompletionInfo.deduplicationPeriod]].
+  * @param submissionId            An identifier for the submission that allows an application
+  *                               to correlate completions to its submissions.


I am not sure I understand these ScalaDoc indentation changes

IntelliJ has made it happen. I actually hadn't realized it did it, it must have been on commit. Shame on me for not reading through my PR before opening it.

I will make sure they're all aligned.

Re-aligned all Scaladoc comments in domain.scala, CompletionInfo.scala, and Update.scala.

victormueller-da · 2021-07-29T09:15:08Z

ledger/ledger-api-domain/src/main/scala/com/digitalasset/ledger/api/domain.scala

+
+  object Logging {
+    implicit def `tagged value to LoggingValue`[T: ToLoggingValue, Tag]: ToLoggingValue[T @@ Tag] =
+      value => value.unwrap
+  }


I do not completely understand the code here yet but I have the feeling this could be quite useful for the json api too 🤔

It says that if we have a type T, a type Tag, and an instance of ToLoggingValue[T], then we also have an instance of ToLoggingValue[T @@ Tag] (which just discards the tag).

For example, if we have a value of type Ref.TransactionId @@ TransactionIdTag, we can convert it to a logging value, because we know how to convert Ref.TransactionId to a logging value.

Happy to discuss further offline if you'd like to understand more.

SamirTalwar added 3 commits July 28, 2021 17:45

logging-entries: Make LoggingEntries a non-case class.

83b21b5

There's no reason for it to need `equals`, etc. CHANGELOG_BEGIN CHANGELOG_END

ledger-api-domain: Convert commands into a logging value.

1b8295d

Instead of having a function, let's use `ToLoggingContext`. This also adds a couple of missing items, and always logs `workflowId`.

participant-state: Convert updates into a logging value.

6252ee4

Instead of having a function, let's use `ToLoggingContext`. This changes some of the logging context structure, but otherwise everything remains the same.

ghost requested review from cocreature, gerolf-da, mziolekda, nmarton-da, rautenrieth-da, remyhaemmerle-da, stefanobaghino-da and tudor-da as code owners July 28, 2021 15:58

ghost requested review from kamil-da, fabiotudone-da, miklos-da and nicu-da July 28, 2021 15:58

kamil-da approved these changes Jul 28, 2021

View reviewed changes

cocreature approved these changes Jul 29, 2021

View reviewed changes

fabiotudone-da approved these changes Jul 29, 2021

View reviewed changes

Make sure Scaladoc is lined up for modified code.

455213f

ghost added the automerge label Jul 29, 2021

victormueller-da reviewed Jul 29, 2021

View reviewed changes

mergify bot merged commit 9c08e4c into main Jul 29, 2021

mergify bot deleted the samir/reduce-logging-entries branch July 29, 2021 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move logging value definitions alongside their objects. #10439

Move logging value definitions alongside their objects. #10439

ghost commented Jul 28, 2021

kamil-da left a comment

kamil-da Jul 28, 2021

cocreature Jul 29, 2021

ghost Jul 29, 2021

cocreature left a comment

cocreature Jul 29, 2021

fabiotudone-da Jul 29, 2021

fabiotudone-da Jul 29, 2021

ghost Jul 29, 2021

ghost Jul 29, 2021 •

edited by ghost

Loading

victormueller-da Jul 29, 2021

ghost Jul 29, 2021

	/** @param party The stable unique identifier of a Daml party.
	/** @param party The stable unique identifier of a Daml party.

Move logging value definitions alongside their objects. #10439

Move logging value definitions alongside their objects. #10439

Conversation

ghost commented Jul 28, 2021

Pull Request Checklist

kamil-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

Choose a reason for hiding this comment

cocreature left a 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

ghost Jul 29, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 29, 2021 •

edited by ghost

Loading