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

feat: include js-ceramic and ceramic-one version in request creation metric #1232

Merged
merged 4 commits into from
Jun 25, 2024
Merged

feat: include js-ceramic and ceramic-one version in request creation metric #1232

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f08de6
feat: include js-ceramic and ceramic-one version in request creation …
stbrody Jun 20, 2024
b78f623
fix test
stbrody Jun 20, 2024
6eddc41
rm integration test
stbrody Jun 20, 2024
e2e3ced
Rename metric
stbrody Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions src/ancillary/anchor-request-params-parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,18 @@ import {
const carFactory = new CARFactory()
carFactory.codecs.add(DAG_JOSE)

export const RequestAnchorParamsV1 = sparse(
const RequestAnchorParamsV3 = sparse(
{
streamId: string.pipe(streamIdAsString),
cid: string.pipe(cidAsString),
timestamp: optional(date),
timestamp: date,
jsCeramicVersion: optional(string),
ceramicOneVersion: optional(string),
},
'RequestAnchorParamsV1'
'RequestAnchorParamsV3'
)

type RequestAnchorParamsV1 = TypeOf<typeof RequestAnchorParamsV1>
export type RequestAnchorParamsV3 = TypeOf<typeof RequestAnchorParamsV3>

const RequestAnchorParamsV2Root = strict({
streamId: uint8array.pipe(streamIdAsBytes),
Expand All @@ -56,12 +58,12 @@ export const RequestAnchorParamsV2 = sparse({

export type RequestAnchorParamsV2 = TypeOf<typeof RequestAnchorParamsV2>

export type RequestAnchorParams = RequestAnchorParamsV1 | RequestAnchorParamsV2
export type RequestAnchorParams = RequestAnchorParamsV3 | RequestAnchorParamsV2

/**
* Encode request params for logging purposes.
*/
export const RequestAnchorParamsCodec = union([RequestAnchorParamsV1, RequestAnchorParamsV2])
export const RequestAnchorParamsCodec = union([RequestAnchorParamsV3, RequestAnchorParamsV2])

export class AnchorRequestCarFileDecoder implements Decoder<Uint8Array, RequestAnchorParamsV2> {
readonly name = 'RequestAnchorParamsV2'
Expand Down Expand Up @@ -96,7 +98,7 @@ export class AnchorRequestParamsParser {
if (req.get('Content-Type') !== 'application/vnd.ipld.car') {
// Legacy requests
Metrics.count(METRIC_NAMES.CTRL_LEGACY_REQUESTED, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these still "legacy" 😅?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to ctrl_json_requested, which is a more descriptive and accurate parallel to ctrl_car_requested

return validate(RequestAnchorParamsV1, req.body)
return validate(RequestAnchorParamsV3, req.body)
} else {
// Next version of anchor requests, using the CAR file format
// TODO: CDB-2212 Store the car file somewhere for future reference/validation of signatures
Expand Down
8 changes: 7 additions & 1 deletion src/controllers/request-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
AnchorRequestParamsParser,
RequestAnchorParams,
RequestAnchorParamsCodec,
RequestAnchorParamsV3,
} from '../ancillary/anchor-request-params-parser.js'
import bodyParser from 'body-parser'
import { type RequestService, RequestDoesNotExistError } from '../services/request-service.js'
Expand Down Expand Up @@ -124,7 +125,12 @@ export class RequestController {

// request was newly created
if (body) {
Metrics.count(METRIC_NAMES.CTRL_NEW_ANCHOR_REQUEST, 1, { source: parseOrigin(req) })
const v3Params = requestParams as RequestAnchorParamsV3
Metrics.count(METRIC_NAMES.CTRL_NEW_ANCHOR_REQUEST, 1, {
source: parseOrigin(req),
jsCeramicVersion: v3Params.jsCeramicVersion,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gvelez17 is this sufficient to get what we want to track what version people are running based on the requests coming into CAS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that for nodes that haven't upgraded, jsCeramicVersion and ceramicOneVersion will be undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add this to getStatusForCid for status requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We certainly could, but seems less useful and there is some cost in bandwidth to sending this on every request.

Copy link
Contributor Author

@stbrody stbrody Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I figured how many requests are created by which versions was a more useful metric than how much polling is happening from which versions

Copy link
Contributor

Choose a reason for hiding this comment

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

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

ceramicOneVersion: v3Params.ceramicOneVersion,
})
return res.status(StatusCodes.CREATED).json(body)
}

Expand Down