Skip to content

Add support for Fortio output format#168

Merged
htuch merged 38 commits intoenvoyproxy:masterfrom
nareddyt:fortio-output-format
Nov 11, 2019
Merged

Add support for Fortio output format#168
htuch merged 38 commits intoenvoyproxy:masterfrom
nareddyt:fortio-output-format

Conversation

@nareddyt
Copy link
Copy Markdown
Contributor

@nareddyt nareddyt commented Oct 18, 2019 •
edited
Loading

Description: Add support for Fortio output format

Testing: Added a unit test the compare the output to a golden file

Docs: Added sample data/reports, added docs to README.md

nareddyt added 11 commits October 17, 2019 17:49
@nareddyt
Initial work on Fortio Output Format
2f4036f 
Added some data files that will be deleted later

Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Add basic fortio proto
2379f61 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Add some more fields
88c6812 
Tested output format:
{
 "labels": "TODO label",
 "starttime": "2019-10-18T01:18:09Z",
 "requestedqps": 1
}

Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Remove unneeded import
661038b 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Disable validation
07a6c7f 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
More fields
9abb017 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
More fields
f39f726 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Fix nullptr
3336bf3 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Fill in all the top-level fields and verified output is correct
79f023a 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Delete some data that is not used anymore
ff250a9 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Fix insertion + exception
eaeca56 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Copy link
Copy Markdown
Contributor Author

nareddyt commented Oct 18, 2019

Current output:

{
 "RetCodes": {
  "200": "60"
 },
 "Labels": "A random label",
 "StartTime": "2019-10-16T00:30:12Z",
 "RequestedQPS": 1,
 "RequestedDuration": "5s",
 "ActualQPS": 0,
 "ActualDuration": 0,
 "NumThreads": 0,
 "Version": "",
 "Exactly": 0,
 "URL": "http://127.0.0.1:10000/",
 "SocketCount": 1
}

Just need to figure out the distribution part and understand the remaining fields that are zero-valued...

@nareddyt
Remove some more fields that are not needed
2c19e11 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Cleanup
a3b864d 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Start working on DurationHistogram
372a0fe 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Mapping of all fields
c526b9c 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Remove first entry from the dataset to make the graph look nicer
68f3690 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Some test data (temporary)
40b01c0 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Better test data
f3cb825 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Copy link
Copy Markdown
Contributor Author

nareddyt commented Oct 18, 2019

This mapping seems to work. Here is an example output:

image

@nareddyt
Fix format
b95cf4e 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Fix tests and docs
0dca025 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Fix initial sample
6a52796 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
oschaaf
oschaaf reviewed Oct 18, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

Looks great generally, some preliminary feedback

@nareddyt
Review comments
a676086 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Move fortio proto to ./transform
4b73fa6 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Add test for output formatter
41838fc 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Fix format
a53dd09 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Remove old test data
c189419 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Add sample data
d210488 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Review comments
4ad87c9 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Review comments envoyproxy#2
22ec3f5 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
oschaaf
oschaaf reviewed Oct 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

This looks great to me. Some final questions/comments from my side.

@oschaaf
Copy link
Copy Markdown
Member

oschaaf commented Oct 29, 2019

Oh, just noticed coverage is unhappy; there may be an easy fix over at https://github.com/envoyproxy/nighthawk/blob/master/test/factories_test.cc#L127 by adding nighthawk::client::OutputFormat::FORTIO to the list of formats being tested.

@nareddyt nareddyt mentioned this pull request Oct 29, 2019
Closed
@nareddyt
Review comments 3
932814f 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt nareddyt requested a review from oschaaf October 29, 2019 22:30
oschaaf
oschaaf previously approved these changes Oct 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM assuming coverage will pass.
@htuch @jplevyak as far as I am concerned this is good to go.
Do you want to take a peek at this?

@nareddyt
Copy link
Copy Markdown
Contributor Author

nareddyt commented Oct 29, 2019

Huh, it looks like clang_tidy failed. I can't figure out the error, any ideas?

@oschaaf
Copy link
Copy Markdown
Member

oschaaf commented Oct 29, 2019

Huh, it looks like clang_tidy failed. I can't figure out the error, any ideas?

I think that's a flake. I've seen it earlier, I think it is related to the CI machine running out of memory.
Just restarted CI. Just filed #187 to track fixing the clang-tidy flakes.

@oschaaf oschaaf requested a review from htuch October 29, 2019 22:43
htuch
htuch suggested changes Oct 31, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Looks good, a few comments; have you verified full coverage of the new code?
/wait

source/client/output_formatter_impl.cc
}
}

throw NighthawkException("Nighthawk result was malformed, contains no "
Copy link
Copy Markdown
Member

@htuch htuch Oct 31, 2019

Choose a reason for hiding this comment

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

This seems an anti-pattern, passing around expected output results as string map values.. should we have more structure here?

Copy link
Copy Markdown
Contributor Author

@nareddyt nareddyt Nov 8, 2019

Choose a reason for hiding this comment

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

@oschaaf perhaps you could look into this? I think this would require changes outside of my PR.

Copy link
Copy Markdown
Member

@oschaaf oschaaf Nov 8, 2019

Choose a reason for hiding this comment

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

I filed #195 to discuss.

Copy link
Copy Markdown
Member

@oschaaf oschaaf Nov 8, 2019

Choose a reason for hiding this comment

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

(and track)

@nareddyt
Merge branch 'master' into fortio-output-format
ece7105 
# Conflicts:
#	README.md
@nareddyt
Add comment about proto-json trick
9a5547a 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Only wrap optional types
2ba9e57 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt
Copy link
Copy Markdown
Contributor Author

nareddyt commented Nov 8, 2019

Ready for review.

Fairly certain that all the code is covered based on @oschaaf's comments. I can't see the coverage artifact on CI for some reason now, it keeps giving me a 404.

@nareddyt nareddyt requested review from htuch and oschaaf November 8, 2019 20:12
@nareddyt
Fix README
fe97b6c 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@oschaaf
Copy link
Copy Markdown
Member

oschaaf commented Nov 8, 2019

A fix/format for the CLI docs was recently introduced for README.md.
Can you run this command? Hopefully that fixes the CI failure:

tools/update_cli_readme_documentation.sh --mode fix

@nareddyt
Fix readme
ef16f64 
Signed-off-by: Teju Nareddy <nareddyt@google.com>
@nareddyt nareddyt force-pushed the fortio-output-format branch from 8c4875d to ef16f64 Compare November 8, 2019 22:57
@oschaaf
Copy link
Copy Markdown
Member

oschaaf commented Nov 8, 2019

The current CI failure is a flake, and my bad. Apparently test_https_h2 isn't stable with asan

htuch
htuch approved these changes Nov 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit 68e5971 into envoyproxy:master Nov 11, 2019
@oschaaf oschaaf mentioned this pull request Jun 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oschaaf oschaaf oschaaf approved these changes

@jplevyak jplevyak Awaiting requested review from jplevyak

+1 more reviewer

@htuch htuch htuch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nareddyt @oschaaf @htuch