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.

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

fix: experimental metrics middleware should use the grpc message constructor name in emitted metrics instead of MiddlewareMessage #993

Merged
merged 6 commits into from
Oct 31, 2023

Conversation

anitarua
Copy link
Contributor

@anitarua anitarua commented Oct 30, 2023

Part of the CloudWatch metrics example work (see #970) in order to use requestType as a dimension on the dashboard graphs.

Also exports ExperimentalMetricsLoggingMiddleware at the top-level of the Node.js SDK and adds null and undefined checks for metrics messages (was running into issues with some middleware messages being undefined while working on #970).

Before this change:

[2023-10-30T22:05:13.777Z] INFO (Momento: ExperimentalMetricsLoggingMiddleware):
{
"numActiveRequestsAtStart":1,
"numActiveRequestsAtFinish":1,
"requestType":"MiddlewareMessage",
"status":0,"startTime":1698703513758,
"requestBodyTime":1698703513759,
"endTime":1698703513776,
"duration":18,
"requestSize":13,
"responseSize":9,
"connectionID":"1"
}

After:

[2023-10-30T22:17:17.087Z] INFO (Momento: ExperimentalMetricsLoggingMiddleware):
{
"numActiveRequestsAtStart":1,
"numActiveRequestsAtFinish":1,
"requestType":"_GetRequest",
"status":0,
"startTime":1698704237069,
"requestBodyTime":1698704237070,
"endTime":1698704237087,
"duration":18,
"requestSize":13,
"responseSize":9,
"connectionID":"3"
}

…tructor name in emitted metrics instead of MiddlewareMessage
@anitarua anitarua marked this pull request as ready for review October 30, 2023 22:31
@anitarua anitarua requested review from cprice404 and a team October 30, 2023 22:46
cprice404
cprice404 previously approved these changes Oct 31, 2023
@anitarua
Copy link
Contributor Author

anitarua commented Oct 31, 2023

@cprice404 Was able to reproduce this test failing locally: CacheClient › createWithEagerConnection returns a client even if it cannot connect

Seems like the if statements I put in were causing that. Will try moving the null check to the MiddlewareMessage.messageLength() function instead I guess based on the error message from the cloudwatch example work.

"TypeError: Cannot read properties of null (reading 'serializeBinary')",
"    at MiddlewareMessage.messageLength (/var/task/index.js:45889:34)",

@anitarua anitarua merged commit 45a0049 into main Oct 31, 2023
13 checks passed
@anitarua anitarua deleted the fix-metrics-message branch October 31, 2023 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants