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

Jump to bottom

DPLT-1044 Expose context method for writing custom Grafana metrics #126

Merged
merged 2 commits into from
Jul 16, 2023
Merged

DPLT-1044 Expose context method for writing custom Grafana metrics #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d79627
feat: Expose context method for writing custom Grafana metrics
morgsmccauley Jul 13, 2023
305f18f
feat: Add `HISTORICAL` dimension to metrics
morgsmccauley Jul 16, 2023
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
4 changes: 4 additions & 0 deletions indexer-js-queue-handler/__snapshots__/metrics.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ exports[`Metrics writes the block height for an indexer function 1`] = `
"Name": "STAGE",
"Value": "dev",
},
{
"Name": "HISTORICAL",
"Value": false,
},
],
"MetricName": "INDEXER_FUNCTION_LATEST_BLOCK_HEIGHT",
"Unit": "None",
Expand Down
18 changes: 14 additions & 4 deletions indexer-js-queue-handler/indexer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export default class Indexer {
functionSubsegment.addAnnotation('indexer_function', function_name);
simultaneousPromises.push(this.writeLog(function_name, block_height, runningMessage));

simultaneousPromises.push(this.deps.metrics.putBlockHeight(indexerFunction.account_id, indexerFunction.function_name, block_height));
simultaneousPromises.push(this.deps.metrics.putBlockHeight(indexerFunction.account_id, indexerFunction.function_name, is_historical, block_height));

const hasuraRoleName = function_name.split('/')[0].replace(/[.-]/g, '_');
const functionNameWithoutAccount = function_name.split('/')[1].replace(/[.-]/g, '_');
Expand Down Expand Up @@ -74,7 +74,7 @@ export default class Indexer {
const vm = new VM({timeout: 3000, allowAsync: true});
const mutationsReturnValue = {mutations: [], variables: {}, keysValues: {}};
const context = options.imperative
? this.buildImperativeContextForFunction(function_name, functionNameWithoutAccount, block_height, hasuraRoleName)
? this.buildImperativeContextForFunction(function_name, functionNameWithoutAccount, block_height, hasuraRoleName, is_historical)
: this.buildFunctionalContextForFunction(mutationsReturnValue, function_name, block_height);

vm.freeze(blockWithHelpers, 'block');
Expand Down Expand Up @@ -225,7 +225,7 @@ export default class Indexer {
};
}

buildImperativeContextForFunction(functionName, functionNameWithoutAccount, block_height, hasuraRoleName) {
buildImperativeContextForFunction(functionName, functionNameWithoutAccount, block_height, hasuraRoleName, is_historical) {
return {
graphql: async (operation, variables) => {
try {
Expand Down Expand Up @@ -254,7 +254,17 @@ export default class Indexer {
},
log: async (log) => {
return await this.writeLog(functionName, block_height, log);
}
},
putMetric: (name, value) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are graphql, set, and log present in buildFunctionalContextForFunction as well as buildImperativeContextForFunction whereas the new putMetric is only present in the latter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good question. We don't use the functional mode anymore. The place we would potentially resurrect it is to parallelize historical processing. That would be for a scenario where we want to execute against blocks in parallel, accumulating mutations to be run, and then running those mutations sequentially. For that sort of disjointed processing metrics might end up weird.
Still, we should probably add it, or we could remove "functional" mode until it's needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't add it because functional is essentially unused now.

Like you said @gabehamilton, metrics could end up weird in functional because of the deferred execution, unless there is someway to pass the timestamp directly 🤔. Probably not worth spending too much time on this though since it's not really used. Happy to remove it in a follow up PR if everyone agrees.

const [accountId, fnName] = functionName.split('/');
return this.deps.metrics.putCustomMetric(
accountId,
fnName,
is_historical,
`CUSTOM_${name}`,
value
);
},
};
}

Expand Down
45 changes: 44 additions & 1 deletion indexer-js-queue-handler/indexer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ mutation _1 { set(functionName: "buildnear.testnet/test", key: "foo2", data: "in
};
await indexer.runFunctions(block_height, functions, false);

expect(metrics.putBlockHeight).toHaveBeenCalledWith('buildnear.testnet', 'test', block_height);
expect(metrics.putBlockHeight).toHaveBeenCalledWith('buildnear.testnet', 'test', false, block_height);
});

test('does not attach the hasura admin secret header when no role specified', async () => {
Expand Down Expand Up @@ -895,6 +895,49 @@ mutation _1 { set(functionName: "buildnear.testnet/test", key: "foo2", data: "in
]);
});

test('allows writing of custom metrics', async () => {
const mockFetch = jest.fn(() => ({
status: 200,
json: async () => ({
errors: null,
}),
}));
const block_height = 456;
const mockS3 = {
getObject: jest.fn(() => ({
promise: () => Promise.resolve({
Body: {
toString: () => JSON.stringify({
chunks: [],
header: {
height: block_height
}
})
}
})
})),
};
const metrics = {
putBlockHeight: () => {},
putCustomMetric: jest.fn(),
};
const indexer = new Indexer('mainnet', { fetch: mockFetch, s3: mockS3, awsXray: mockAwsXray, metrics });

const functions = {};
functions['buildnear.testnet/test'] = {code:`
context.putMetric('TEST_METRIC', 1)
`};
await indexer.runFunctions(block_height, functions, true, { imperative: true });

expect(metrics.putCustomMetric).toHaveBeenCalledWith(
'buildnear.testnet',
'test',
true,
'CUSTOM_TEST_METRIC',
1
);
});

// The unhandled promise causes problems with test reporting.
// Note unhandled promise rejections fail to proceed to the next function on AWS Lambda
test.skip('Indexer.runFunctions() continues despite promise rejection, unable to log rejection', async () => {
Expand Down
14 changes: 11 additions & 3 deletions indexer-js-queue-handler/metrics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ export default class Metrics {
this.namespace = namespace;
}

putBlockHeight(accountId, functionName, height) {
putBlockHeight(accountId, functionName, isHistorical, height) {
return this.putCustomMetric(accountId, functionName, isHistorical, "INDEXER_FUNCTION_LATEST_BLOCK_HEIGHT", height);
}

putCustomMetric(accountId, functionName, isHistorical, metricName, value) {
return this.cloudwatch
.putMetricData({
MetricData: [
{
MetricName: "INDEXER_FUNCTION_LATEST_BLOCK_HEIGHT",
MetricName: metricName,
Dimensions: [
{
Name: "ACCOUNT_ID",
morgsmccauley marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -26,9 +30,13 @@ export default class Metrics {
Name: "STAGE",
Value: process.env.STAGE,
},
{
Name: "HISTORICAL",
Value: isHistorical,
},
],
Unit: "None",
morgsmccauley marked this conversation as resolved.
Show resolved Hide resolved
Value: height,
Value: value,
},
],
Namespace: this.namespace,
Expand Down
2 changes: 1 addition & 1 deletion indexer-js-queue-handler/metrics.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('Metrics', () => {
};
const metrics = new Metrics('test', cloudwatch);

await metrics.putBlockHeight('morgs.near', 'test', 2);
await metrics.putBlockHeight('morgs.near', 'test', false, 2);

expect(cloudwatch.putMetricData).toBeCalledTimes(1);
expect(cloudwatch.putMetricData.mock.calls[0]).toMatchSnapshot()
Expand Down