feat: include js-ceramic and ceramic-one version in request creation metric #1232
Conversation
| const v3Params = requestParams as RequestAnchorParamsV3 | ||
| Metrics.count(METRIC_NAMES.CTRL_NEW_ANCHOR_REQUEST, 1, { | ||
| source: parseOrigin(req), | ||
| jsCeramicVersion: v3Params.jsCeramicVersion, |
There was a problem hiding this comment.
@gvelez17 is this sufficient to get what we want to track what version people are running based on the requests coming into CAS?
There was a problem hiding this comment.
Note that for nodes that haven't upgraded, jsCeramicVersion and ceramicOneVersion will be undefined
There was a problem hiding this comment.
Should we also add this to getStatusForCid for status requests?
There was a problem hiding this comment.
We certainly could, but seems less useful and there is some cost in bandwidth to sending this on every request.
There was a problem hiding this comment.
I figured how many requests are created by which versions was a more useful metric than how much polling is happening from which versions
There was a problem hiding this comment.
it should be - we don't really want more than 2 version parameters per node, I would think. these are the key ones. If something else changes we can try to enforce bumping the minor version of js-ceramic to recognize it
| @@ -1,684 +0,0 @@ | |||
| import 'reflect-metadata' | |||
There was a problem hiding this comment.
This test has been flaky for a while but is failing consistently on this PR. I'm pretty sure it's because I changed the timestamp in anchor requests to be required instead of optional, and we are still using a very old version of js-ceramic in the CAS repo which probably aren't sending the timestamp. Upgrading the CAS to the newest version of js-ceramic is non-trivial and frankly doesn't seem worth it just for test code, now that the CAS doesn't use ceramic in its actual production code.
When I first wrote this test we had very little in the way of test coverage that the CAS was working and that ceramic and the CAS were playing well together. Nowadays we have a lot more test coverage and have the ceramic-cas integration tested in several other places. Because of that I think maintaining this test is becoming more trouble than it's worth.
FYI @smrz2001
| // Legacy requests | ||
| Metrics.count(METRIC_NAMES.CTRL_LEGACY_REQUESTED, 1) |
There was a problem hiding this comment.
Are these still "legacy" 😅?
There was a problem hiding this comment.
yeah it's not, it's now back to being the main expected path. I thought about renaming it but then it will break continuity with the existing metric name. @gvelez17 curious your thoughts on if this is worth renaming now or not?
There was a problem hiding this comment.
hm I don't think i named this, i do not see cas_server_ctrl_legacy_requested_total on any of the primary cas dashboards (tho i didn't do a comprehensive search). I think, since a number of metrics are in flux anyway, I would not worry about historical continuity right now, go ahead and rename it appropriately!
There was a problem hiding this comment.
renamed to ctrl_json_requested, which is a more descriptive and accurate parallel to ctrl_car_requested
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1232 +/- ##
==========================================
Coverage ? 78.16%
==========================================
Files ? 36
Lines ? 1649
Branches ? 264
==========================================
Hits ? 1289
Misses ? 360
Partials ? 0 ☔ View full report in Codecov by Sentry. |
When using ceramic-one as the underlying event storage the "ceramicOneVersion" will be reported as something like
"ceramic-one/0.19.0". When using ipfs/kubo, it looks something like"kubo/0.18.1/"This is the CAS-side of the change, the corresponding js-ceramic change is in ceramicnetwork/js-ceramic#3246