Closed gvelez17 closed 2 months ago
gvelez17
commented
2 months ago ok - @christianlavoie i changed it to 'enabled = true' which i agree is clearer to the end user. The bit I didn't like is the logic making them enabled by default now is in two places, in the daemon config file and the start, for the case when there is no metrics section at all. Added a comment, so hopefully we will notice this if we switch it later to non-default.
gvelez17
commented
2 months ago ok @stbrody added a doc file and a log message referring to it. My preference is to keep the documentation IN the repo with the code, so if we change the code later each version of code has a corresponding document.
The current message is as follows (if node metrics are enabled, which they are now by default)
[2024-06-19T22:05:42.714Z] IMPORTANT: Ceramic API running on 0.0.0.0:7007'
[2024-06-19T22:05:42.758Z] IMPORTANT: Publishing Node Metrics to the Ceramic Network. To learn more, including how to disable publishing, please see the NODE_METRICS.md file for your branch, e.g. https://github.com/ceramicnetwork/js-ceramic/blob/develop/docs-dev/NODE_METRICS.mdI would like to merge this now into develop; i can also write a more extensive blog post on it after it is deployed on dev-unstable and maybe testnet.
gvelez17
commented
2 months ago ugh bad merge (I thought i'd rebased nicely!) recreated here https://github.com/ceramicnetwork/js-ceramic/pull/3244
gvelez17
commented
2 months ago a few small suggestions/questions, mostly around adding clarity to what the different metrics represent, and future-proofing us a little bit for the switch to ceramic-one.
Also, I don't see the code that adds the log message you mentioned in your last comment?
Dunno what happened in the merge, its here https://github.com/ceramicnetwork/js-ceramic/pull/3239/files#diff-29a806cda83d5289fde2f9b08500d1f2335980307791376095ed3d7c600da98eR364
but this is so messy now i recreated it here https://github.com/ceramicnetwork/js-ceramic/pull/3244
in that PR the message is here
closing this one now, asked your review on that one. It has the latest with the committed suggestions you made @stbrody !
Description
As we move towards more decentralization, publishing metrics on the ceramic network becomes more important - so defaulting it to on. Will notify folks in an announcement and post.
How Has This Been Tested?
Describe the tests that you ran to verify your changes. Provide instructions for reproduction.
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.