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

[JSON-API] Add db metrics & response construction metrics #10068

Merged
merged 1 commit into from
Jun 22, 2021
Merged

[JSON-API] Add db metrics & response construction metrics #10068

merged 1 commit into from
Jun 22, 2021

Conversation

realvictorprm
Copy link
Contributor

Solves the last bits of #10020 except the For command submissions the duration of the request to the ledger. point.

changelog_begin

  • [JSON-API] The database operations (regardless of in-memory or postgres) contain now metrics for fetching contracts by id or key (seperate metrics foreach)
  • [JSON-API] The time for a repsonse payload construction of a request is now also tracked in a metric

changelog_end

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.

@realvictorprm
[JSON-API] Add db metrics & response construction metrics
4b9088f 
changelog_begin

- [JSON-API] The database operations (regardless of in-memory or postgres) contain now metrics for fetching contracts by id or key (seperate metrics foreach)
- [JSON-API] The time for a repsonse payload construction of a request is now also tracked in a metric

changelog_end
@realvictorprm realvictorprm added the component/json-api HTTP JSON API label Jun 21, 2021
stefanobaghino-da
stefanobaghino-da approved these changes Jun 21, 2021
View reviewed changes
ledger-service/http-json/src/main/scala/com/digitalasset/http/Endpoints.scala
)
}
.recover { case NonFatal(e) =>
httpResponseError(ServerError(e.description))
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that this comes from existing code, but should we also take advantage of this chance to log this exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am generally against logging anything that is reported by proper channels already; it's a good way to leak sensitive data.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are recovering from a server error here, I think it makes sense for the information to be logged on the server. I would argue it would probably make less sense to return the full description to the client (e.g. if the connection to the database fails I'd be likely be more interested to have it logged rather than reported to the client). Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you can honestly say "don't report the description to the client at all in this case", then it makes sense. Otherwise, we're pointing in one direction and jumping in the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the errors caught here are meaningful to be reported in full only to the operator of the server, then yes. There are cases in which both a detailed report to the user and logging to the advantage of the operator could make sense. Not sure of what could be thrown here. It looks to me like here most exceptions would be interesting from an operational point of view but less so from a user perspective, but I'll let you judge that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This escalated quickly 😂

Thing is, how do we know whether the information is either best only for the user or the operator of the server?

Copy link
Contributor Author

@realvictorprm realvictorprm Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I want to postpone this to a separate pr and better discuss this in slack or in an gh issue

@realvictorprm
Copy link
Contributor Author

If there are no further objections I would merge this

@stefanobaghino-da
Copy link
Contributor

If there are no further objections I would merge this

If you have an approval, go for it. 🙂

If there are open points on an approved PR it means they can be addressed in a separate PR, as long as it's done in a timely fashion.

Our approach is well described here.

@realvictorprm
Copy link
Contributor Author

Then LGTM 😁 First work PR to be merged today and second PR overall to be merged today 😁

@realvictorprm realvictorprm merged commit 0391f35 into main Jun 22, 2021
@realvictorprm realvictorprm deleted the http-json/even-moar-metrics branch June 22, 2021 08:34
azure-pipelines bot pushed a commit that referenced this pull request Jun 23, 2021
release 1.15.0-snapshot.20210622.7213.0.d867d904
dfcf81c 
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.

@S11001001 is in charge of this release.

Commit log:
```
d867d90 define query offset semantics solely in terms of #9847 per-query offsets (#10071)
1f5aa44 [JSON-API] Add metrics for ledger command submission timing (#10076)
7eb2ce7 update compat versions for 1.15.0-snapshot.20210615.7169.0.adeba206 (#10028)
c44f337 Simplify speedy benchmark (#10079)
58b1c4e Speedy: Refactor contract Id/Key callback (#10033)
8558c73 Upgrade canton to 0.25.0 based on daml 1.14.0 (#10074)
e4c1c58 Run scenarios in off-ledger machine (#10070)
31a76a4 allow CI pools to use any zone (#10069)
0391f35 [JSON-API] Add db metrics & response construction metrics (#10068)
ea089ad clean up local VSCode after release (#10065)
39a5890 fix 9_functional redirect (#10067)
fb757d8 Handle visibility outside of speedy (#10056)
b796695 Add per-query offsets to HTTP-JSON API (#9847)
ab8bac5 [In-memory fan-out] BuffersUpdater subscribes from the ledger end (#10037)
e458529 [JSON-API] Add moar timing metrics (#10045)
e12a449 s/DABL/Daml Hub/ (#10062)
a205d0e Enriched conformance tests for transactions (trees) streams (#9977)
b98ad91 Bump test sha (#10055)
f3106c3 I9996 value limits it fails oracle (#10027)
febca5d Use ScenarioRunner.submit in Daml Script (#10053)
3113702 index json-api signatories and observers on Oracle (#9895)
cb3a42a update NOTICES file (#10047)
2f51869 Allow constraints in any position in data-deps. (#10049)
7dfa36f pass supportedJdbcDriverNames implicitly, and only to what actually needs it (#10036)
0fbc1ee H2 support for append only schema (part2) [DPP-394] (#10030)
cd2fda6 Fix broken package.json config (#10044)
10ab05f Fix vsce publishing (#10043)
090dd95 rotate release duty after 1.14.0-snapshot.20210615.7169.0.adeba206 (#10019)
eee484b Implemented in-memory buffers prunning in BaseLedger (#9936)
b96639e Do not terminate streams when no end-offset given in `ledger-api-bench-tool` [DPP-422] (#10035)
2b915e9 Split scenario & ledger execution (#10039)
591176c Remove SqlSequence, cleanup code (#10042)
8ea240f Release SDK 1.14.0 (#10041)
3532460 LF: Structure Preprocessing Errors (#10013)
1852830 [JSON-API] Concurrent query etc. metrics (#10031)
321c4da test different template-ID frequencies in the "mega" perf scenario (#10016)
993591e truncate party name for 64 chars for multi-valued submitters to avoid… (#10000)
7a2a349 Log context of all updates written to the database (#10010)
4005c84 release 1.15.0-snapshot.20210615.7169.0.adeba206 (#10022)
d1aa256 Force css-what resolution in compiler/daml-extension & fully regenera… (#10025)
0fbfd99 DPP-394 h2 support for append only schema (#10009)
32d70cf Revert "release 1.14.0-snapshot.20210615.7169.0.adeba206 (#10018)" (#10023)
cfee5a2 [JSON-API] Add information from the jwtpayload to the logging context (#9995)
fe63825 release 1.14.0-snapshot.20210615.7169.0.adeba206 (#10018)
```
Changelog:
```
- [JSON-API] Timing metrics which measure how long the processing of a command submission request takes on the ledger are now available

- [JSON-API] The database operations (regardless of in-memory or postgres) contain now metrics for fetching contracts by id or key (seperate metrics foreach)
- [JSON-API] The time for a repsonse payload construction of a request is now also tracked in a metric

[HTTP/JSON API] The streaming query endpoint now accepts to override the
offset for each query using the `offset` field. You can read more
about it on the documentation.
- [JSON-API] Timing metrics are now available for party management, package management, command submission and query endpoints.
- [JSON-API] Also added a timing metric for parsing and decoding of incoming json payloads
- [JSON API] The Oracle database schema has changed; if using
  ``--query-store-jdbc-config``, you must rebuild the database by adding
  ``,createSchema=true``.  See #9895.
- [JSON-API] The metrics which describe the amount of these concurrent events is now available:
	- running http request
	- command submissions
	- package allocations
	- party allocations
[metrics] Limit size of multi party metrics to ease consumption
For every update in the index db log the full context at the INFO level.
- [JSON API] For applicable requests actAs, readAs, applicationId & ledgerId are included in the log context
```

CHANGELOG_BEGIN
CHANGELOG_END
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 approved these changes

@cocreature cocreature Awaiting requested review from cocreature

@gerolf-da gerolf-da Awaiting requested review from gerolf-da

@mziolekda mziolekda Awaiting requested review from mziolekda

@nmarton-da nmarton-da Awaiting requested review from nmarton-da

@rautenrieth-da rautenrieth-da Awaiting requested review from rautenrieth-da

@S11001001 S11001001 Awaiting requested review from S11001001

@tudor-da tudor-da Awaiting requested review from tudor-da

Assignees
No one assigned
Labels
component/json-api HTTP JSON API
Projects
None yet
Milestone
No milestone
Development

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