search

envoyproxy / nighthawk

L7 (HTTP/HTTPS/HTTP2/HTTP3) performance characterization tool
Apache License 2.0
361 stars 81 forks source link

Mismatched type for fortio response #932

Open Revolyssup opened 2 years ago

Revolyssup commented 2 years ago

Title: Mismatched type for fortio response causes failure in interconversion

Description: The message format in protobuff here https://github.com/envoyproxy/nighthawk/blob/f42add9bb5c71a5652db2f9471dda357b96d5ee2/api/client/transform/fortio.proto#L31 mismatches the expected datatype by fortio here https://github.com/fortio/fortio/blob/d2a2d42f4f17df4c09e608353546ebd0d443747d/periodic/periodic.go#L221 on the field RequestedQPS

This is causing Unmarshalling to fail while conversion from one to another. Reproduction steps:

Include sample requests, environment, etc. All data and inputs required to reproduce the bug.

Note: If there are privacy concerns, sanitize the data prior to sharing.

Logs:

Include the Nighthawk logs.

Note: If there are privacy concerns, sanitize the data prior to sharing.

Call Stack:

If the Envoy binary is crashing, a call stack is required. Please refer to the Bazel Stack trace documentation.

leecalcote commented 2 years ago

Thank you for filing this, @Revolyssup

mum4k commented 2 years ago

Thank you for the report @Revolyssup.

Can you help me understand - are you saying that the correct type is int as exported by Nighthawk, but fortio incorrectly takes that in as a string? Or are we saying that fortio is correct and we should adjust the Nighthawk's fortio transform to export QPS as string instead of int?

leecalcote commented 1 year ago

@MUzairS15 will you offer these details? Let's work through this item.

MUzairS15 commented 1 year ago

yes, @mum4k fortio exports QPS as string https://pkg.go.dev/fortio.org/fortio/periodic#RunnerResults. Another example is RetCodes https://github.com/fortio/fortio/blob/v1.55.0/fhttp/httprunner.go#L37 Fortio expects map[int]int were as nighthawk transforms it as map'[string]int https://github.com/envoyproxy/nighthawk/blob/0fb828685228ab42bd7d3338eae157f759a6d68a/api/client/transform/fortio.proto#L49

The Transformation needs to be updated.

mum4k commented 1 year ago

Thank you for explaining that @MUzairS15, given this just needs a modification in the interface between Nighthawk and Fortio, we should be ok to make this change without breaking anyone. I will go ahead and mark this issue as "help wanted", that way anyone with the cycles can pick it up and contribute the fix.