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

Ledger API: Add healthcheck endpoints. #3573

Merged
merged 25 commits into from
Nov 22, 2019
Merged

Ledger API: Add healthcheck endpoints. #3573

merged 25 commits into from
Nov 22, 2019

Conversation

SamirTalwar
Copy link
Contributor

Step 1 of implementing #2733.

Compliant with the GRPC Health Checking Protocol.

These are always SERVING for now.

I'd like to make the Sandbox report NOT_SERVING when, for example, the PostgreSQL connection is lost, but this will require a bunch of testing and a lot more code, so I'd like to stage the work.

Also forthcoming: documentation on how to use this endpoint (spoiler: grpc-health-probe), and how to make your own ledger report its health accurately.

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, 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.

@SamirTalwar
ledger: Delete ledger/API.md.
62784ca
@SamirTalwar
grpc-definitions: Delete health_service.proto
5cd1d2a 
We can use the version in io.grpc:grpc-services instead.
@SamirTalwar
sandbox: Fix warnings in ApiServices flagged by IntelliJ.
f5d33dd
@SamirTalwar
sandbox: Implement a dummy grpc.health.v1.Health.Check endpoint.
2a74761
@SamirTalwar
sandbox: Implement a dummy grpc.health.v1.Health.Watch endpoint.
105f5da
@SamirTalwar
sandbox: Drop repeated elements from grpc.health.v1.Health.Watch.
3f5cfd7
@SamirTalwar
sandbox: Wrap the HealthService in basic tests.
b00dbf5
@SamirTalwar
sandbox: Stop streaming the server health too.
3cc71be
@SamirTalwar
ledger-api-test-tool: Health check tests.
2b9d8df
@SamirTalwar
Add a changelog entry for the health check endpoints.
c5fea5d 
CHANGELOG_BEGIN

- [Ledger API] Add healthcheck endpoints, conforming to the
  `GRPC Health Checking Protocol <https://github.com/grpc/grpc/blob/master/doc/health-checking.md>`_.
  It is always ``SERVING`` for now.

- [DAML Ledger Integration Kit] Add conformance test coverage for the
  ``grpc.health.v1.Health`` service.

CHANGELOG_END
stefanobaghino-da
stefanobaghino-da approved these changes Nov 21, 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.

Mostly LGTM, can you have a close look at the comments in TimeBoundObserver before merging?

ledger/sandbox/src/main/scala/com/digitalasset/ledger/server/apiserver/ApiServices.scala Show resolved Hide resolved
ledger/sandbox/src/main/scala/com/digitalasset/platform/sandbox/health/DropRepeated.scala Outdated Show resolved Hide resolved
ledger/sandbox/src/main/scala/com/digitalasset/platform/sandbox/health/DropRepeated.scala Outdated Show resolved Hide resolved
ledger/sandbox/src/main/scala/com/digitalasset/platform/sandbox/health/package.scala Outdated Show resolved Hide resolved
...sandbox/src/test/suite/scala/com/digitalasset/platform/sandbox/health/DropRepeatedSpec.scala Outdated Show resolved Hide resolved
...andbox/src/test/suite/scala/com/digitalasset/platform/sandbox/health/HealthServiceSpec.scala Outdated Show resolved Hide resolved
ledger/test-common/src/main/scala/com.digitalasset.platform.testing/TimeBoundObserver.scala Outdated Show resolved Hide resolved
SamirTalwar and others added 8 commits November 21, 2019 16:26
@SamirTalwar @stefanobaghino-da
Apply suggestions from code review
0533fc9 
Co-Authored-By: Stefano Baghino <43749967+stefanobaghino-da@users.noreply.github.com>
@SamirTalwar
sandbox: Use AkkaBeforeAndAfterAll in the HealthServiceSpec.
90bfb63 
In an attempt to get it working on CI.
@SamirTalwar
sandbox: Change dropRepeated to DropRepeated().
f296630 
Keep it in one file.
@SamirTalwar
test-common: Use Delayed.by in TimeBoundObserver.
f202dc2
@SamirTalwar
test-common: Close the source when TimeBoundObserver completes.
594c34d
@SamirTalwar
Merge branch 'master' into ledger/sandbox/healthchecks
a3fc679
@SamirTalwar
./fmt.sh
ba65e23 
That'll teach me not to `--no-verify` just because it's a merge commit.
@SamirTalwar
sandbox: Inline HealthService.suppress.
bcfd74b 
At some point it was being used twice.
@gerolf-da gerolf-da requested a review from mziolekda November 21, 2019 16:38
gerolf-da
gerolf-da requested changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@gerolf-da gerolf-da left a comment

Choose a reason for hiding this comment

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

Thanks. One comment we should talk about is Protobuf-Java vs. ScalaPB service implementation.

ledger/sandbox/src/main/scala/com/digitalasset/platform/sandbox/health/HealthService.scala Outdated
class HealthService(watchThrottleFrequency: FiniteDuration = 1.second)(
implicit materializer: Materializer,
executionContext: ExecutionContext
) extends HealthGrpc.HealthImplBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if we could stick with the scalapb rpc implementation for services, otherwise we start having a mix of how services are implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@stefanobaghino-da stefanobaghino-da mentioned this pull request Nov 21, 2019
Merged
6 tasks
@SamirTalwar
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@SamirTalwar
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

gerolf-da
gerolf-da reviewed Nov 22, 2019
View reviewed changes
ledger-api/rs-grpc-testing-utils/BUILD.bazel
"src/main/scala/**/*.scala",
]),
tags = [
"maven_coordinates=com.digitalasset.ledger-api:rs-grpc-testing-utils:__VERSION__",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay, the release check actually worked 🎉

@mergify mergify bot merged commit 0ff716d into master Nov 22, 2019
@mergify mergify bot deleted the ledger/sandbox/healthchecks branch November 22, 2019 14:02
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

@gerolf-da gerolf-da gerolf-da approved these changes

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

@mziolekda mziolekda Awaiting requested review from mziolekda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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