Fortio output: add missing fields & correct some of the output fields by oschaaf · Pull Request #226 · envoyproxy/nighthawk

oschaaf · 2019-12-11T12:21:12Z

PR Description

Define NH version in the code in a canonical place.
Update the fortio output formatter - add missing version field
Consume the new version information in various places to make
--version output of executables sensible.
Implement a couple of missing fields in the Fortio output
Amend a couple of fields for correctness

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

Prerequisites to merging this

Move RELEASE_PROCEDURE.md into its own PR
Land API: Add extension information the Node message. envoy#9301
[WIP] Update the NH Envoy dependency so we can use ^^: Update Envoy #261
Update this PR to leverage the structured versioning that now is available
Fix missing stuff in the Fortio output, punt what cannot be completed yet to TODO/issues and/or task lists here.
amend inaccurate things
~~Discuss that the label field in Fortio output also contains machine name, we set it to Nighthawk. Check which one is preferrable~~.
Get consensus on what we want to show for the concurrency value in the Fortio visualization, discussion
use experimental pool features (h1 connection re-use strategy, h2 multiple connection support)

- Define NH version in the code in a canonical place. - Update the fortio output formatter - add missing version field - Add RELEASE_PROCEDURE.md - Consume the new version information in various places to make --version output of executables sensible. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k · 2019-12-11T15:18:20Z

RELEASE_PROCEDURE.md

+- Update [version_history.md](docs/root/version_history.md). 
+  - Make sure breaking changes are explicitly called out.
+  - Ensure the release notes are complete.
+- Tag the release in git using the current version number


Would it help if we include example commands for some of these steps?

Yes I think so; I added some placeholder commands for @htuch to review (he has been doing the actual releases).

mum4k · 2019-12-11T15:18:55Z

RELEASE_PROCEDURE.md

@@ -0,0 +1,7 @@
+# Release procedure
+
+- Update [version_history.md](docs/root/version_history.md). 


(optional) Not sure if worth it (depends on how often this happens). Would it make sense to add a script that will automate the version update and the version "Bump" noted below?

Yes, filed #228

mum4k · 2019-12-11T15:19:57Z

include/nighthawk/common/version.h

+const int MAJOR_VERSION{0};
+const int MINOR_VERSION{3};
+
+class Globals {


Curious - why wrap it in a class?

Considering its generic name - won't this attract more possibly unrelated methods?

Bad naming indeed; I renamed the class to VersionUtils. WDYT?

mum4k · 2019-12-11T15:20:57Z

source/client/output_formatter_impl.cc


  // TODO(#182): Not needed but nice to have, displays in the UI
  fortio_output.set_labels("");
+  fortio_output.set_version("0.0");


Don't we want to set this to the actual version?

In this particular test, IMHO I think "0.0" is fine as we target testing the output formatting; and not the versioning mechanics; But having said that I feel an end-to-end test would contribute value - especially at release time.
So I punted this to #228
Does that sound good?

Don't we want to set this to the actual version?

I'm sorry, I had another look at this, and I somehow misunderstood your comment here.
That is correct, and this is fixed now.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf

@mum4k thanks; I addressed your feedback in 6f23597

oschaaf · 2019-12-11T19:45:30Z

RELEASE_PROCEDURE.md

+- Update [version_history.md](docs/root/version_history.md). 
+  - Make sure breaking changes are explicitly called out.
+  - Ensure the release notes are complete.
+- Tag the release in git using the current version number


Yes I think so; I added some placeholder commands for @htuch to review (he has been doing the actual releases).

oschaaf · 2019-12-11T19:45:46Z

RELEASE_PROCEDURE.md

@@ -0,0 +1,7 @@
+# Release procedure
+
+- Update [version_history.md](docs/root/version_history.md). 


Yes, filed #228

oschaaf · 2019-12-11T20:52:35Z

include/nighthawk/common/version.h

+const int MAJOR_VERSION{0};
+const int MINOR_VERSION{3};
+
+class Globals {


Bad naming indeed; I renamed the class to VersionUtils. WDYT?

oschaaf · 2019-12-11T20:55:17Z

source/client/output_formatter_impl.cc


  // TODO(#182): Not needed but nice to have, displays in the UI
  fortio_output.set_labels("");
+  fortio_output.set_version("0.0");


In this particular test, IMHO I think "0.0" is fine as we target testing the output formatting; and not the versioning mechanics; But having said that I feel an end-to-end test would contribute value - especially at release time.
So I punted this to #228
Does that sound good?

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

htuch · 2019-12-11T23:24:02Z

RELEASE_PROCEDURE.md

+# Release procedure
+
+- Update [version_history.md](docs/root/version_history.md). 
+  - Make sure breaking changes are explicitly called out.


Ideally we do these as we go, rather than a major effort at release time. The experience form Envoy has been that this scales better.

I'll make sure we mention that here - though maybe we could also have a pre-merge reviewer checklist that captures this; when someone about to perform a release read it here it's probably too late?

Sure. Could be useful for new maintainers, +1

Sure. Could be useful for new maintainers, +1

Filed #230, let me know if you have any more things that should go in there.

htuch · 2019-12-11T23:24:37Z

RELEASE_PROCEDURE.md

+- Tag the release in git using the current version number:
+```bash
+git tag -a "v0.3" -m "Release v0.3"
+git push origin v0.3


Do we need to do the tagging? If we do a GitHub tagged release, does it generate these for us?

Those currently exist, so if that is what you did earlier, then yes :-) I'll amend

htuch · 2019-12-11T23:25:12Z

include/nighthawk/common/version.h

+namespace Nighthawk {
+
+const int MAJOR_VERSION{0};
+const int MINOR_VERSION{3};


Do you want full semantic major.minor.patch style versions?

Lets go for that, I'll amend

htuch · 2019-12-11T23:26:11Z

api/client/transform/fortio.proto


  string URL = 12;
+
+  string Version = 13;


You could make this a structured type, @yanavlasov is introducing this in envoyproxy/envoy#9301

The structured versions have been merged BTW

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2019-12-12T08:42:19Z

@mum4k @htuch I addressed feedback and iterated on the release procedure;
As we can only land this after envoyproxy/envoy#9301 goes in, it's a nice time to
discuss this some more, so please let me know what you think! I tried to capture
some of the things we did in the earlier releases, as well as added some ideas.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2019-12-14T13:58:04Z

This is now in flux for a bit, because I noticed a couple of issues as wel as some fields missing in the output. As this is blocked on waiting for Envoy to land the structured versioning proto definition I may as well go for getting the output sorted completely while I'm at it.

RELEASE_PROCEDURE.md

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2019-12-15T14:34:11Z

/cc @nareddyt there's some changes going on with respect to the Fortio output generation here, looping you in, in case you'd like to have a look at this.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-01-09T12:21:38Z

Note fore reviewers: It's better to land #261 first, as I merged that into this one so I could take support for structured versioning onwards.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Update Envoy to 29b30911dbfb3f9760efeb28238ceac36e1a1a23 It is worth noting this upgrades Envoy proto api usage to v3alpha Brings in the structured versioning API, which we need in #226 Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

jplevyak · 2020-01-09T23:25:20Z

api/client/transform/fortio.proto


  string URL = 12;
+
+  string Version = 13;


The structured versions have been merged BTW

jplevyak · 2020-01-09T23:29:21Z

source/client/output_formatter_impl.cc

+  fortio_output.set_jitter(output.options().has_jitter_uniform());
+  fortio_output.set_runtype("HTTP");
+
+  // The h2 pool doesn't offer supper for multiple connections here. So we must ignore the


s/supper/support/

jplevyak · 2020-01-13T21:36:56Z

source/client/output_formatter_impl.cc

-  for (int i = 0; i < nh_stat.percentiles().size(); i++) {
-
+  const int percentiles_size = nh_stat.percentiles().size();
+  for (int i = 0; i < percentiles_size; i++) {


You can use a range-based for loop here.

jplevyak · 2020-01-13T21:39:27Z

source/common/version_info.cc

+}
+
+const envoy::config::core::v3alpha::BuildVersion& VersionInfo::buildVersion() {
+  static const auto* result = new envoy::config::core::v3alpha::BuildVersion(


Can you use CONSTRUCT_ON_FRIST_USE here as well?

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-01-13T22:56:59Z

@jplevyak feedback addressed in 13b5222

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k

Just two documentation nits, please let me know if you prefer to merge and address later.

mum4k · 2020-01-14T23:46:15Z

source/common/version_info.h

+ */
+class VersionInfo {
+public:
+  static const std::string& version();


Can we document these public methods?

mum4k · 2020-01-14T23:46:53Z

api/client/transform/fortio.proto


  string URL = 12;
+
+  string Version = 13;


(optional) Unsure what role this proto plays. If it is "public" by any means we should improve the documentation of these fields. We can leave this to a follow-up PR.

Followed up on the requested doc comments in 77a7eb2 over at #265

mum4k · 2020-01-14T23:48:59Z

Discussed offline, we will address the documentation nits in another PR. This one going in will unblock two other PRs.

oschaaf added 3 commits December 11, 2019 13:20

Fix ambiguous include path

7c267eb

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Swap ot fmt and use absl::StrCat instead

88f2e03

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k self-requested a review December 11, 2019 15:17

mum4k requested changes Dec 11, 2019

View reviewed changes

oschaaf mentioned this pull request Dec 11, 2019

Add a script(s) to facilitate release tasks #228

Open

Review feedback

6f23597

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf commented Dec 11, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fortio-version-field

a8e8f50

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

htuch reviewed Dec 11, 2019

View reviewed changes

oschaaf added 2 commits December 12, 2019 08:36

Merge remote-tracking branch 'upstream/master' into fortio-version-field

aa28310

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback + iterate on release procedure

dbab3b4

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into fortio-version-field

5cc8a79

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf requested a review from jplevyak December 13, 2019 22:36

oschaaf added 4 commits December 14, 2019 12:34

Consolidate version in the output

315be10

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Back out accidentally comitted file

640ea06

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix achieved qps / duration / connections / qps

8e93a74

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Remove TODO(envoyproxy#186)

e9306fa

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf changed the title ~~Fortio output: add version field~~ Fortio output: add version field, resolve some inaccuracies Dec 14, 2019

Add missing fields to the proto definition

4568133

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf changed the title ~~Fortio output: add version field, resolve some inaccuracies~~ Fortio output: add missing fields & correct some of the output fields Dec 14, 2019

oschaaf commented Dec 14, 2019

View reviewed changes

RELEASE_PROCEDURE.md Outdated Show resolved Hide resolved

oschaaf added 2 commits December 15, 2019 15:17

Define & set min/max/sum/avg/stdev/percentiles

6076b70

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Back out RELEASE_PROCEDURE.md

8a6bee0

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added 3 commits December 15, 2019 16:39

Fix error ratio

3a1f428

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Stub the jitter flag with a TODO

98b91e0

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add doc comments for durationToSeconds()

ba703cb

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add minimal structured versioning support

5f28389

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added 7 commits January 9, 2020 13:24

Fix command to avoid check_format flagging code

24782ea

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix includes & small tweaks

f7b8eaf

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Amend according to clang-tidy suggestion

111aee5

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into fortio-version-field

812df3e

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into envoy-dep-update-8

8239486

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into envoy-dep-update-8

23dc8ae

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge branch 'envoy-dep-update-8' into fortio-version-field

838dcdd

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into fortio-version-field

2840070

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed blocked A PR that is blocked by prerequisites. waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jan 10, 2020

oschaaf mentioned this pull request Jan 10, 2020

Investigate full re-use of Envoy's VersionInfo class (and backing infra) #267

Open

oschaaf added 2 commits January 10, 2020 22:29

Fix TODO comment / link to issue number

6b341fc

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fortio conn. count fix for experimental h2 pool

f150f6a

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf mentioned this pull request Jan 11, 2020

Add tracking of response body-size and header-size #265

Merged

8 tasks

oschaaf added this to the 0.3 milestone Jan 11, 2020

jplevyak previously approved these changes Jan 13, 2020

View reviewed changes

Address review feedback

13b5222

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf dismissed jplevyak’s stale review via 13b5222 January 13, 2020 22:56

jplevyak approved these changes Jan 14, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fortio-version-field

de3fb47

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k self-assigned this Jan 14, 2020

mum4k approved these changes Jan 14, 2020

View reviewed changes

mum4k merged commit c88bc4e into envoyproxy:master Jan 14, 2020

oschaaf mentioned this pull request Jun 24, 2020

Support fortio output format #155

Closed

		@@ -0,0 +1,7 @@
		# Release procedure

		- Update [version_history.md](docs/root/version_history.md).

Conversation

oschaaf commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Prerequisites to merging this

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Dec 12, 2019

Uh oh!

oschaaf commented Dec 14, 2019

Uh oh!

Uh oh!

oschaaf commented Dec 15, 2019

Uh oh!

oschaaf commented Jan 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Jan 13, 2020

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

oschaaf commented Dec 11, 2019 •

edited

Loading