Skip to content

thrift_proxy: refactor thrift router stats to create named stats/stat names once#18586

Merged
mattklein123 merged 3 commits intoenvoyproxy:mainfrom
fishcakez:thrift-router-stats
Oct 13, 2021
Merged

thrift_proxy: refactor thrift router stats to create named stats/stat names once#18586
mattklein123 merged 3 commits intoenvoyproxy:mainfrom
fishcakez:thrift-router-stats

Conversation

@fishcakez
Copy link
Copy Markdown
Contributor

@fishcakez fishcakez commented Oct 12, 2021

Commit Message: thrift_proxy: refactor thrift router stats to create named stats/stat names once
Additional Description: The Router::RequestOwner constructor is called once per upstream request. The constructor creates stats and adds stat names to symbol tables. These are always the same named counters/histograms and stat names. When using payload passthrough and only the router filter the RequestOwner constructor is 20% +/- 1.5% of the time spent in the ThriftProxy namespace. Therefore refactor the stats out to their own class and create in the filter factory, and pass a constant reference to the router filter and through to other router classes' constructors.

perf flamegraph where Envoy::Extensions::NetworkFilters::ThriftProxy::Rout on the wide left tower is the Router and RequestOwner constructors.

Screen Shot 2021-10-12 at 9 12 44 AM

Risk Level: low
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

… names once

Signed-off-by: James Fish <jfish@pinterest.com>
@fishcakez fishcakez requested a review from zuercher as a code owner October 12, 2021 16:27
Signed-off-by: James Fish <jfish@pinterest.com>
@rgs1
Copy link
Copy Markdown
Member

rgs1 commented Oct 12, 2021

cc: @jmarantz in case there are other places where we could do this too.

@fishcakez
Copy link
Copy Markdown
Contributor Author

Unrelated CI failure:

ERROR: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/com_github_lyft_protoc_gen_star/BUILD.bazel:3:11: GoCompilePkg external/com_github_lyft_protoc_gen_star/protoc-gen-star.a [for host] failed: (Exit 1): builder failed: error executing command 
  (cd /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy && \
  exec env - \
    CGO_ENABLED=1 \
    GOARCH=amd64 \
    GOOS=linux \
    GOPATH='' \
    GOROOT=external/go_sdk \
    GOROOT_FINAL=GOROOT \
    PATH=/opt/llvm/bin:/usr/bin:/bin \
  bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src external/com_github_lyft_protoc_gen_star/artifact.go -src external/com_github_lyft_protoc_gen_star/ast.go -src external/com_github_lyft_protoc_gen_star/build_context.go -src external/com_github_lyft_protoc_gen_star/comment.go -src external/com_github_lyft_protoc_gen_star/debug.go -src external/com_github_lyft_protoc_gen_star/docs.go -src external/com_github_lyft_protoc_gen_star/entity.go -src external/com_github_lyft_protoc_gen_star/enum.go -src external/com_github_lyft_protoc_gen_star/enum_value.go -src external/com_github_lyft_protoc_gen_star/extension.go -src external/com_github_lyft_protoc_gen_star/field.go -src external/com_github_lyft_protoc_gen_star/field_type.go -src external/com_github_lyft_protoc_gen_star/field_type_elem.go -src external/com_github_lyft_protoc_gen_star/file.go -src external/com_github_lyft_protoc_gen_star/generator.go -src external/com_github_lyft_protoc_gen_star/init_option.go -src external/com_github_lyft_protoc_gen_star/message.go -src external/com_github_lyft_protoc_gen_star/method.go -src external/com_github_lyft_protoc_gen_star/module.go -src external/com_github_lyft_protoc_gen_star/name.go -src external/com_github_lyft_protoc_gen_star/node.go -src external/com_github_lyft_protoc_gen_star/oneof.go -src external/com_github_lyft_protoc_gen_star/package.go -src external/com_github_lyft_protoc_gen_star/parameters.go -src external/com_github_lyft_protoc_gen_star/persister.go -src external/com_github_lyft_protoc_gen_star/post_process.go -src external/com_github_lyft_protoc_gen_star/proto.go -src external/com_github_lyft_protoc_gen_star/service.go -src external/com_github_lyft_protoc_gen_star/source_code_info.go -src external/com_github_lyft_protoc_gen_star/wkt.go -src external/com_github_lyft_protoc_gen_star/workflow.go -arc 'github.com/golang/protobuf/proto=github.com/golang/protobuf/proto=bazel-out/host/bin/external/com_github_golang_protobuf/proto/proto.x' -arc 'github.com/golang/protobuf/protoc-gen-go/plugin=github.com/golang/protobuf/protoc-gen-go/plugin=bazel-out/host/bin/external/io_bazel_rules_go/proto/wkt/compiler_plugin_go_proto.x' -arc 'github.com/golang/protobuf/protoc-gen-go/descriptor=github.com/golang/protobuf/protoc-gen-go/descriptor=bazel-out/host/bin/external/io_bazel_rules_go/proto/wkt/descriptor_go_proto.x' -importpath github.com/lyft/protoc-gen-star -p github.com/lyft/protoc-gen-star -package_list bazel-out/host/bin/external/go_sdk/packages.txt -o bazel-out/host/bin/external/com_github_lyft_protoc_gen_star/protoc-gen-star.a -x bazel-out/host/bin/external/com_github_lyft_protoc_gen_star/protoc-gen-star.x -gcflags '' -asmflags '')
Execution platform: @envoy_build_tools//toolchains:rbe_linux_clang_libcxx_platform
compilepkg: missing strict dependencies:
	/b/f/w/external/com_github_lyft_protoc_gen_star/init_option.go: import of "github.com/spf13/afero"
	/b/f/w/external/com_github_lyft_protoc_gen_star/persister.go: import of "github.com/spf13/afero"
No dependencies were provided

Copy link
Copy Markdown
Member

@rgs1 rgs1 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, thanks! Just one nit about saving an extra const ref when we can just get it from parent, but not blocking on that.

cc: @davinci26 if you are up for another review + merge.

@jmarantz
Copy link
Copy Markdown
Contributor

Thanks for the heads up, @rgs1 -- this looks like a good refactor. I'm not aware of any that remain at this point, but then I was not aware of this one :)

Probably there is still some low-hanging fruit in other extensions.

Signed-off-by: James Fish <jfish@pinterest.com>
@fishcakez
Copy link
Copy Markdown
Contributor Author

I merged upstream/main to give CI another go :/

Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

Over to @davinci26 for merging once CI is happy.

@rgs1
Copy link
Copy Markdown
Member

rgs1 commented Oct 12, 2021

/assign davinci26

@mattklein123 mattklein123 merged commit ec33096 into envoyproxy:main Oct 13, 2021
@fishcakez fishcakez deleted the thrift-router-stats branch October 14, 2021 03:04
@fishcakez fishcakez restored the thrift-router-stats branch October 15, 2021 02:42
@fishcakez fishcakez deleted the thrift-router-stats branch October 15, 2021 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants