Skip to content

Cannot expose HealthChecks over the event bus #49

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

Open
afloarea opened this issue Nov 14, 2022 · 5 comments
Open

Cannot expose HealthChecks over the event bus #49

afloarea opened this issue Nov 14, 2022 · 5 comments
Labels

Comments

@afloarea
Copy link
Contributor

Questions

"Do not use this issue tracker to ask questions, instead use one of these channels. Questions will likely be closed without notice." - This might not be the time or the place, but I've noticed that the Discord invitation on vertx.io is invalid and/or expired. Is there some other way to ask questions?

Version

4.3.4

Context

I encountered an exception while trying to expose health checks over the event bus.

Do you have a reproducer?

Yes, I have the following test (the middle part is copied from vertx.io docs):

    @Test
    void test(Vertx vertx, VertxTestContext testContext) {

        HealthChecks healthChecks = HealthChecks.create(vertx);
        healthChecks.register("test", promise -> promise.complete(Status.OK()));

        vertx.eventBus().consumer("health",
            message -> healthChecks.checkStatus()
                .onSuccess(message::reply)
                .onFailure(err -> message.fail(0, err.getMessage())));

        vertx.eventBus().<CheckResult>request("health", "check").map(Message::body)
            .onComplete(testContext.succeeding(checkResult -> {
                Assertions.assertTrue(checkResult.getUp());
                testContext.completeNow();
            }));
    }

Steps to reproduce

Run the test. It fails and logs this exception:

00:30:26 [vert.x-eventloop-thread-0] ERROR ContextBase:? Unhandled exception
java.lang.IllegalArgumentException: No message codec for type: class io.vertx.ext.healthchecks.CheckResult
	at io.vertx.core.eventbus.impl.CodecManager.lookupCodec(CodecManager.java:119)
	at io.vertx.core.eventbus.impl.EventBusImpl.createMessage(EventBusImpl.java:254)
	at io.vertx.core.eventbus.impl.MessageImpl.createReply(MessageImpl.java:122)
	at io.vertx.core.eventbus.impl.MessageImpl.reply(MessageImpl.java:104)
	at io.vertx.core.eventbus.Message.reply(Message.java:81)
        at ...

Extra

I noticed that both Status and CheckResult are annotated with @DataObject, however none of them can be sent over the event bus (fails with No message codec for type, also I haven't added vertx-codegen as a dependency).
I also tried a workaround by creating a custom MessageCodec, but looking at the CheckResult class, it does not have any method/constructor which takes a JsonObject (am I missing something? 🤔 I thought @DataObject sort of implies this)

Lastly, with some minimal guidance I am open to contributing a fix for this.
Thank you for your time, attention and for the Vert.x project 🙏 🤘

@afloarea afloarea added the bug label Nov 14, 2022
@gaol
Copy link
Member

gaol commented Nov 15, 2022

@afloarea can you try to replace:
vertx.eventBus().<CheckResult>request("health", "check").map(Message::body)

to -->

vertx.eventBus().<JsonObject>request("health", "check").map(Message::body)

?

@vietj
Copy link
Contributor

vietj commented Nov 15, 2022

I think we could fix the data object to get this constructor and make the data objects easier to create from json, now the constructor for json object is not mandary.

@afloarea
Copy link
Contributor Author

Thanks for the quick reply

@gaol if I put JsonObject instead of CheckResult I still get the same exception No message codec for type: class io.vertx.ext.healthchecks.CheckResult

@vietj Are you suggesting to add a constructor with JsonObject to the CheckResult class? or all annotated @DataObjects?

Also, it's not very clear to me what @DataObject means. Just that it can be easier to convert to and/or from JsonObject (for codegen)? or does it also imply that you can send/receive the annotatated types over the event bus out of the box?

@vietj
Copy link
Contributor

vietj commented Nov 16, 2022

the one we need.

it means it can be converted back and forth to JSON, but that is a thing for our code generator and not for the event-bus that requires introspectability of the type which data object do not provide.

@afloarea
Copy link
Contributor Author

I had a look over the CheckResult conversion to json and I can make a PR to add a json-to-CheckResult conversion constructor. It's not a 1-to-1 mapping though and I see the following problems:

  • CheckResult seems to contain one of 3 most of the time:
  1. List<CheckResult>
  2. id + Status
  3. id + failure

Going from JSON to CheckResult means identifying which of the 3 cases and the only way to select between 2 and 3 (if you try to map to Status or to failure) seems to check for "data": {"cause": ..."} in the json

  • How do you go from json to Throwable failure field in the CheckResult?

Another thing is, even if this change were to be implemented, the documentation on the website would still suggest that you can reply directly with a CheckResult to an event bus message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants