fix: correctly use controller when registering interest and correctly use node's ID when publishing metrics. #3273
Conversation
packages/core/src/recon.ts
Outdated
| @@ -169,11 +169,10 @@ export class ReconApi extends Observable<ReconEventFeedResponse> implements IRec | |||
| } | |||
| try { | |||
| const headers = { 'Content-Type': 'application/json' } | |||
| const body = { ...(controller && { controller }) } | |||
| await this.#sendRequest(this.#url + `/ceramic/interests/model/${model.toString()}`, { | |||
| const query = controller ? `controller=${controller}` : '' | |||
There was a problem hiding this comment.
Do we have a canonical way of constructing query strings for URLs? This string manipulation is not ideal.
packages/core/src/recon.ts
Outdated
| const body = { ...(controller && { controller }) } | ||
| await this.#sendRequest(this.#url + `/ceramic/interests/model/${model.toString()}`, { | ||
| const query = controller ? `controller=${controller}` : '' | ||
| await this.#sendRequest(this.#url + `/ceramic/interests/model/${model.toString()}?${query}`, { |
There was a problem hiding this comment.
huh, should this be considered an issue with the API that C1 is exposing? This is a POST request, why is it using URL query parameters instead of taking its args in the POST body? Maybe the thing to do is change it in C1 instead of here?
CC @dav1do
There was a problem hiding this comment.
ah, yeah. that's actually the old one.. we have a newer one POST /interests that behaves like that
There was a problem hiding this comment.
Query params are not exclusive to get requests. I don't have a strong opinion either way just updated the client to match the server.
There was a problem hiding this comment.
@dav1do Thanks for the link to the new endpoint. I changed the code to use the new endpoint and manually tested. Working well...
correctly use node's ID when publishing metrics.
nathanielc
force-pushed
the
fix/node-metrics-publish-bugs
branch
from
103e198
to
e3b5ba0
Compare
Description
In testing the node metrics publish I found two bugs:
This fixes both issues.
The controller needs to be a query parameter not a POST body object.
The
ipfsId.publicKeywas empty I do not know why, however theipfsId.idfield was set correctly and works as expected.How Has This Been Tested?
Manually tested, starting a node. Previously the node saw errors everytime it tried to publish metrics. Additionally I directly inspected the interest and was able to see it was for all controllers not just itself. I have manually verified the interest is now for the specific controller.
PR checklist
Before submitting this PR, please make sure:
References:
Please list relevant documentation (e.g. tech specs, articles, related work etc.) relevant to this change, and note if the documentation has been updated.