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

[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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions ledger-service/http-json/src/main/scala/com/digitalasset/http/ContractsService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.daml.ledger.api.refinements.{ApiTypes => lar}
import com.daml.ledger.api.v1.active_contracts_service.GetActiveContractsResponse
import com.daml.ledger.api.{v1 => api}
import com.daml.logging.{ContextualizedLogger, LoggingContextOf}
import com.daml.metrics.{Metrics, Timed}
import com.daml.util.ExceptionOps._
import scalaz.Id.Id
import scalaz.std.option._
Expand Down Expand Up @@ -70,7 +71,8 @@ class ContractsService(
parties: OneAnd[Set, domain.Party],
contractLocator: domain.ContractLocator[LfValue],
)(implicit
lc: LoggingContextOf[InstanceUUID]
lc: LoggingContextOf[InstanceUUID],
metrics: Metrics,
): Future[Option[domain.ResolvedContractRef[LfValue]]] =
contractLocator match {
case domain.EnrichedContractKey(templateId, key) =>
Expand All @@ -87,7 +89,8 @@ class ContractsService(
jwtPayload: JwtPayload,
contractLocator: domain.ContractLocator[LfValue],
)(implicit
lc: LoggingContextOf[InstanceUUID]
lc: LoggingContextOf[InstanceUUID],
metrics: Metrics,
): Future[Option[domain.ActiveContract[JsValue]]] =
contractLocator match {
case domain.EnrichedContractKey(templateId, contractKey) =>
Expand All @@ -102,20 +105,30 @@ class ContractsService(
templateId: TemplateId.OptionalPkg,
contractKey: LfValue,
)(implicit
lc: LoggingContextOf[InstanceUUID]
): Future[Option[domain.ActiveContract[JsValue]]] =
search.toFinal
.findByContractKey(SearchContext[Id, Option](jwt, parties, templateId), contractKey)
lc: LoggingContextOf[InstanceUUID],
metrics: Metrics,
): Future[Option[domain.ActiveContract[JsValue]]] = {
Timed.future(
metrics.daml.HttpJsonApi.dbFindByContractKey,
search.toFinal
.findByContractKey(SearchContext[Id, Option](jwt, parties, templateId), contractKey),
)
}

private[this] def findByContractId(
jwt: Jwt,
parties: OneAnd[Set, lar.Party],
templateId: Option[domain.TemplateId.OptionalPkg],
contractId: domain.ContractId,
)(implicit
lc: LoggingContextOf[InstanceUUID]
): Future[Option[domain.ActiveContract[JsValue]]] =
search.toFinal.findByContractId(SearchContext(jwt, parties, templateId), contractId)
lc: LoggingContextOf[InstanceUUID],
metrics: Metrics,
): Future[Option[domain.ActiveContract[JsValue]]] = {
Timed.future(
metrics.daml.HttpJsonApi.dbFindByContractId,
search.toFinal.findByContractId(SearchContext(jwt, parties, templateId), contractId),
)
}

private[this] def search: Search = SearchDb getOrElse SearchInMemory

Expand Down
40 changes: 23 additions & 17 deletions ledger-service/http-json/src/main/scala/com/digitalasset/http/Endpoints.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,22 +471,27 @@ class Endpoints(

private def httpResponse[A: JsonWriter](
result: ET[domain.SyncResponse[A]]
): Future[HttpResponse] = {
val fa: Future[Error \/ (JsValue, StatusCode)] =
result.flatMap { x =>
either(SprayJson.encode1(x).map(y => (y, x.status)).liftErr(ServerError))
}.run
fa.map {
case -\/(e) =>
httpResponseError(e)
case \/-((jsVal, status)) =>
HttpResponse(
entity = HttpEntity.Strict(ContentTypes.`application/json`, format(jsVal)),
status = status,
)
}.recover { case NonFatal(e) =>
httpResponseError(ServerError(e.description))
}
)(implicit metrics: Metrics): Future[HttpResponse] = {
Timed.future(
metrics.daml.HttpJsonApi.responseCreationTimer,
result
.flatMap { x =>
either(SprayJson.encode1(x).map(y => (y, x.status)).liftErr(ServerError))
}
.run
.map {
case -\/(e) =>
httpResponseError(e)
case \/-((jsVal, status)) =>
HttpResponse(
entity = HttpEntity.Strict(ContentTypes.`application/json`, format(jsVal)),
status = status,
)
}
.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

},
)
}

private[http] def data(entity: RequestEntity): Future[String] =
Expand Down Expand Up @@ -581,7 +586,8 @@ class Endpoints(
jwtPayload: JwtWritePayload,
reference: domain.ContractLocator[LfValue],
)(implicit
lc: LoggingContextOf[JwtPayloadTag with InstanceUUID with RequestID]
lc: LoggingContextOf[JwtPayloadTag with InstanceUUID with RequestID],
metrics: Metrics,
): Future[Error \/ domain.ResolvedContractRef[ApiValue]] =
contractsService
.resolveContractReference(jwt, jwtPayload.parties, reference)
Expand Down
6 changes: 6 additions & 0 deletions ledger/metrics/src/main/scala/com/daml/metrics/Metrics.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,12 @@ final class Metrics(val registry: MetricRegistry) {
// Meters how long parsing and decoding of an incoming json payload takes
val incomingJsonParsingAndValidationTimer: Timer =
registry.timer(Prefix :+ "incoming_json_parsing_and_validation_timing")
// Meters how long the construction of the response json payload takes
val responseCreationTimer: Timer = registry.timer(Prefix :+ "response_creation_timing")
// Meters how long a find by contract key database operation takes
val dbFindByContractKey: Timer = registry.timer(Prefix :+ "db_find_by_contract_key_timing")
// Meters how long a find by contract id database operation takes
val dbFindByContractId: Timer = registry.timer(Prefix :+ "db_find_by_contract_id_timing")
// Meters http requests throughput
val httpRequestThroughput: Meter = registry.meter(Prefix :+ "http_request_throughput")
// Meters how many websocket connections are currently active
Expand Down