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

daml-lf/data: Optionally truncate party names in structured logs. [KVL-996] #10163

Merged
merged 6 commits into from
Jul 1, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jul 1, 2021

The party name can grow quite long, so we offer ledger implementors the opportunity to truncate it in structured log output.

Unfortunately, because we use Logback through the global LoggerFactory, there is no place to inject logging configuration. This means we also need to use global, mutable state to configure logging output. I have added a LoggingConfiguration class+object in Daml-LF Data, which may not be the best place, but I can't think of a better one right now. I suggest we leave it there until it has reason to grow, at which point we may want to move it.

I moved the logging implicits to a new package to reduce the number of explicit transitive dependencies.

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.

The party name can grow quite long, so we offer ledger implementors the
opportunity to truncate it in structured log output.

Unfortunately, because we use Logback through the global
`LoggerFactory`, there is no place to inject logging configuration. This
means we also need to use global, mutable state to configure logging
output. I have added a `LoggingConfiguration` class+object in Daml-LF
Data, which may not be the best place, but I can't think of a better
one right now. I suggest we leave it there until it has reason to grow,
at which point we may want to move it.

CHANGELOG_BEGIN
CHANGELOG_END
Invalid input should not break the request at this point. No assertions.
This avoids the transitive dependency issue most of the time.
Again, reduces the need for transitively depending on _logging-entries_.
Copy link
Contributor

@oliverse-da oliverse-da left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you, @SamirTalwar-DA

@ghost ghost added the automerge label Jul 1, 2021
@mergify mergify bot merged commit e7e8a57 into main Jul 1, 2021
@mergify mergify bot deleted the samir/daml-lf/data/party-logging branch July 1, 2021 16:50
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.

4 participants