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

aperture-js: Use binary serialization for the aperture.check_response attribute #2654

Open
krdln opened this issue Sep 20, 2023 · 1 comment

Comments

@krdln
Copy link
Contributor

krdln commented Sep 20, 2023

Json serialization of timestamps has some issues (they're serialized as objects and not strings, as per protojson format) and @harjotgill created a workaround for that, which converts all timestamp fields manually.

// HACK: Change timestamps to ISO strings since the protobufjs library uses it in a different format
// Issue: https://github.com/protobufjs/protobuf.js/issues/893
// PR: https://github.com/protobufjs/protobuf.js/pull/1258
// Current timestamp type: https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/timestamp.proto
const localCheckResponse = this.checkResponse as any;
localCheckResponse.start = this.protoTimestampToJSON(
this.checkResponse.start,
);
localCheckResponse.end = this.protoTimestampToJSON(
this.checkResponse.end,
);
localCheckResponse.waitTime = this.protoDurationToJSON(
this.checkResponse.waitTime,
);
// Walk through individual decisions and convert waitTime fields,
// then add to localCheckResponse, preserving immutability.
if (this.checkResponse.limiterDecisions) {
const decisions = this.checkResponse.limiterDecisions.map(
(decision) => {
return {
...decision,
waitTime: this.protoDurationToJSON(decision.waitTime),
};
},
);
localCheckResponse.limiterDecisions = decisions;
}
this.span.setAttribute(
CHECK_RESPONSE_LABEL,
JSON.stringify(localCheckResponse),
);

An alternative workaround would be to utilize binary serialization instead of json. We can do it because metricsprocessor accepts aperture.check_response as json but also as base64-encoded protowire. This could even result in a performance improvement as a side-effect.

Note: This requires swapping out the generator, as the current one doesn't provide anything like serializeBinary / encode, etc.

Note: If this requires some more effort, let's stay with current workaround.

@DariaKunoichi
Copy link
Contributor

Generated new typescript files from proto using https://www.npmjs.com/package/grpc_tools_node_protoc_ts?activeTab=readme#how-to-use
Currently have a problem with importing those added files - code pushed to branch js_new_proto_gen.
Leaving this task for now.

@DariaKunoichi DariaKunoichi removed their assignment Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants