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.

Sign up for GitHub

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

Jump to bottom

SQL diagnostics #2662

Merged
merged 8 commits into from
Aug 30, 2019
Merged

SQL diagnostics #2662

merged 8 commits into from
Aug 30, 2019

Conversation

gerolf-da
Copy link
Contributor

@gerolf-da gerolf-da commented Aug 26, 2019 •
edited
Loading

This PR adds some TRACE level logging for how long database operations are taking.
Also updates the Postgres JDBC driver to the latest release.
I don't intend to merge this before #2425.

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
  • Add a line to the release notes, if appropriate
  • 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.

@gerolf-da gerolf-da added the component/ledger Sandbox and Ledger API label Aug 26, 2019
@gerolf-da gerolf-da force-pushed the postgres-diagnostics branch from 61d56f2 to 9f52ccf Compare August 26, 2019 13:13
@gerolf-da gerolf-da changed the title Postgres diagnostics SQL diagnostics Aug 26, 2019
@gerolf-da gerolf-da force-pushed the postgres-diagnostics branch from 9f52ccf to cd55dfd Compare August 26, 2019 13:22
stefanobaghino-da
stefanobaghino-da reviewed Aug 26, 2019
View reviewed changes
Copy link
Contributor

@stefanobaghino-da stefanobaghino-da left a comment

Choose a reason for hiding this comment

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

Lots of nitpicking, probably having two implementations of the runQuery method based on the logging level is the only one I would push a bit to implement. 🙂 Yay for tracking! 🎉

dependencies.yaml Show resolved Hide resolved
...c/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/dao/PostgresLedgerDao.scala Outdated Show resolved Hide resolved
...c/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/dao/PostgresLedgerDao.scala Outdated Show resolved Hide resolved
...c/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/dao/PostgresLedgerDao.scala Outdated Show resolved Hide resolved
...c/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/dao/PostgresLedgerDao.scala Outdated Show resolved Hide resolved
...c/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/dao/PostgresLedgerDao.scala Outdated Show resolved Hide resolved
...c/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/dao/PostgresLedgerDao.scala Outdated Show resolved Hide resolved
...x/src/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/util/DbDispatcher.scala Outdated Show resolved Hide resolved
...x/src/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/util/DbDispatcher.scala Outdated Show resolved Hide resolved
...ox/src/main/scala/com/digitalasset/platform/sandbox/stores/ledger/sql/util/SqlExecutor.scala Outdated Show resolved Hide resolved
gerolf-da added a commit that referenced this pull request Aug 27, 2019
@gerolf-da
Make description a by-name parameter.
310fa9d 
This avoids building up the string if trace logging
is not enabled (i.e. most of the time).

Addresses #2662 (review)
gerolf-da added a commit that referenced this pull request Aug 27, 2019
@gerolf-da
Log the "raw" contract id string, and not the wrapping case class.
f0a779e 
Addresses #2662 (review)
@rautenrieth-da
Copy link
Contributor

How will this interact with MeteredLedgerDao? That class already collects metrics on the duration of DAO operations (but on a more coarse level). Is that class superseded by the trace level logging? Or are we going to keep both of them?

@gerolf-da
Copy link
Contributor Author

Good point, @rautenrieth-da. We keep both. MeteredLedgerDao is better to see trends and can be turned on in regular operation, whereas the trace logging is for concrete debugging of database performance issues.

gerolf-da added a commit that referenced this pull request Aug 28, 2019
@gerolf-da
Make description a by-name parameter.
f5a2ab5 
This avoids building up the string if trace logging
is not enabled (i.e. most of the time).

Addresses #2662 (review)
gerolf-da added a commit that referenced this pull request Aug 28, 2019
@gerolf-da
Log the "raw" contract id string, and not the wrapping case class.
ad471ed 
Addresses #2662 (review)
@gerolf-da gerolf-da force-pushed the postgres-diagnostics branch from 7ae9da2 to c8059f3 Compare August 28, 2019 12:23
@gerolf-da gerolf-da marked this pull request as ready for review August 28, 2019 12:57
gerolf-da added a commit that referenced this pull request Aug 29, 2019
@gerolf-da
Make description a by-name parameter.
56b3fca 
This avoids building up the string if trace logging
is not enabled (i.e. most of the time).

Addresses #2662 (review)
gerolf-da added a commit that referenced this pull request Aug 29, 2019
@gerolf-da
Log the "raw" contract id string, and not the wrapping case class.
7d1e1b8 
Addresses #2662 (review)
@gerolf-da gerolf-da force-pushed the postgres-diagnostics branch from 6a5aea3 to 16d6913 Compare August 29, 2019 15:04
gerolf-da added a commit that referenced this pull request Aug 30, 2019
@gerolf-da
Make description a by-name parameter.
166424e 
This avoids building up the string if trace logging
is not enabled (i.e. most of the time).

Addresses #2662 (review)
gerolf-da added a commit that referenced this pull request Aug 30, 2019
@gerolf-da
Log the "raw" contract id string, and not the wrapping case class.
d295959 
Addresses #2662 (review)
@gerolf-da gerolf-da force-pushed the postgres-diagnostics branch from 5c15121 to 7b77365 Compare August 30, 2019 07:11
gerolf-da added a commit that referenced this pull request Aug 30, 2019
@gerolf-da
Make description a by-name parameter.
e8d4cd5 
This avoids building up the string if trace logging
is not enabled (i.e. most of the time).

Addresses #2662 (review)
gerolf-da added a commit that referenced this pull request Aug 30, 2019
@gerolf-da
Log the "raw" contract id string, and not the wrapping case class.
a4b2c15 
Addresses #2662 (review)
@gerolf-da gerolf-da force-pushed the postgres-diagnostics branch from 7b77365 to b94df1d Compare August 30, 2019 08:37
@gerolf-da
Use proper names for the DB connection pools.
b40ebf0
@gerolf-da
Add some trace logging for DB operations.
73aac35 
This does not capture timing of individual statements but
rather the time for "units of work".
For example "lookup contract" doesn't mean only loading
a single row from the contracts table, but also 2 additional
queries for looking up witnesses and divulgences.

This is not a problem, because this is trace level logging
that helps us debug problems and shouldn't be made sense of by
users at this stage.
@gerolf-da
Make description a by-name parameter.
97b6669 
This avoids building up the string if trace logging
is not enabled (i.e. most of the time).

Addresses #2662 (review)
@gerolf-da
Log the "raw" contract id string, and not the wrapping case class.
38789fe 
Addresses #2662 (review)
@gerolf-da
Wrap logging in isTraceEnabled.
8596097
@gerolf-da
scalafmt
98148aa
@gerolf-da gerolf-da force-pushed the postgres-diagnostics branch from a89a822 to fe3fd28 Compare August 30, 2019 08:59
rautenrieth-da
rautenrieth-da reviewed Aug 30, 2019
View reviewed changes
Copy link
Contributor

@rautenrieth-da rautenrieth-da left a comment

Choose a reason for hiding this comment

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

The change looks good, apart from the change to unreleased.rst - was that intentional?

unreleased.rst
@@ -13,12 +13,8 @@ HEAD — ongoing
file to be a pointer to the source directory of the DAML code contained in a project relative to
the project root. This is breaking projects, where the ``source`` field of the project is pointing
to a non-toplevel location in the source code directory structure.
+ [Sandbox] The sandbox now properly sets the connection pool properties ``minimumIdle``, ``maximumPoolSize``, and ``connectionTimeout``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these lines really be deleted?

Copy link
Contributor Author

@gerolf-da gerolf-da Aug 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yes, a previous automerge messed up the release notes and this is fixing the mess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah now I see, these are notes that were already moved to the actual release notes. 👍

@rautenrieth-da rautenrieth-da self-requested a review August 30, 2019 11:39
@gerolf-da gerolf-da merged commit ecb506e into master Aug 30, 2019
@gerolf-da gerolf-da deleted the postgres-diagnostics branch August 30, 2019 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanobaghino-da stefanobaghino-da stefanobaghino-da left review comments

@rautenrieth-da rautenrieth-da rautenrieth-da approved these changes

Assignees
No one assigned
Labels
component/ledger Sandbox and Ledger API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gerolf-da @rautenrieth-da @stefanobaghino-da