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

[output] ability to natively convert nighthawk output to fortio output #179

Merged
merged 3 commits into from
Sep 20, 2021

Conversation

DelusionalOptimist
Copy link
Contributor

@DelusionalOptimist DelusionalOptimist commented Sep 19, 2021

Signed-off-by: Rudraksh Pareek [email protected]

Description

This PR fixes #

Notes for Reviewers
This PR:

  • Removes the dependency on nighthawk_output_transform
  • Makes transformation go native.
  • Makes transformation more efficient and aligned with nighthawk's capability.
  • Removes hacks.

Why do we need this?
Unlike nighthawk_client (CLI) nighthawk_service (gRPC service) doesn't offer output transformation to other formats. See envoyproxy/nighthawk#152
The histogram that is constructed for performance tests is (at least for now) based on fortio's json output so we need the ability to transform.
The code here uses the same logic and the output structure that is implemented by the nighthawk_output_transformer

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Rudraksh Pareek <[email protected]>
Signed-off-by: Rudraksh Pareek <[email protected]>
Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Lgtm

@oschaaf
Copy link

oschaaf commented Sep 19, 2021

Naive question probably .. but I wonder: why not wrap the nighthawk_transform binary to piggy back on this functionality being maintained at 0 cost?

@DelusionalOptimist
Copy link
Contributor Author

Yes @oschaaf, that is a much feasible solution. In fact, this is what we're doing right now. The results are transformed using the binary shipped in Meshery server's image. However, both the Nighthawk binaries are pretty big in size (600 MB) and also need shared libraries. As we're trying to reduce meshery server's image size (1.7 GB rn) and also move to a lightweight base image, we probably need to get these out. The nighthawk_service would be soon running in a different container and this PR is an attempt to help out with transformation.
Another reason is to avoid running shell commands directly. But I don't know if this on its own is a big enough reason.

@warunicorn19
Copy link
Member

@DelusionalOptimist is this PR good to merge?

@DelusionalOptimist
Copy link
Contributor Author

@oschaaf since you're here and I'm not much sure if any thoughts have been put on this in the past, do you know why the nighthawk binaries published here are much bigger than the ones that are shipped in nighthawk-dev image? There seems to be a significant difference.

@oschaaf
Copy link

oschaaf commented Sep 20, 2021

@oschaaf since you're here and I'm not much sure if any thoughts have been put on this in the past, do you know why the nighthawk binaries published here are much bigger than the ones that are shipped in nighthawk-dev image? There seems to be a significant difference.

It is a bit of a guess, but over at envoyproxy/nighthawk there's https://github.com/envoyproxy/nighthawk/blob/main/.bazelrc and https://github.com/envoyproxy/nighthawk/blob/main/.bazelversion.
Is there an equivalent of those being used for building the binaries that get published here?

@warunicorn19 warunicorn19 merged commit 63e2362 into layer5io:master Sep 20, 2021
@welcome
Copy link

welcome bot commented Sep 20, 2021

Thanks for your contribution to the Layer5 community! 🎉

Congrats!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Performance management
Development

Successfully merging this pull request may close these issues.

4 participants