Census client filter: use current span and tags #21249
Conversation
ajamato
requested review from
gnossen,
jtattermusch,
nicolasnoble and
veblush
as code owners
|
|
ajamato
force-pushed
the
grpc_census_tags
branch
2 times, most recently
from
7d85a3e
to
f0946b1
Compare
|
Would it be possible for someone to add labels to this PR: "lang/c++", "release notes: yes". Got myself into some git trouble, but think I am in a good state now. Though I don't understand the submodule changes included in this PR. Is there a way to revert these? I didn't see anything in the contribution guide about this. Did some script make changes when I ran tests locally? |
ajamato
changed the title
| @@ -81,11 +81,21 @@ def grpc_deps(): | |||
| actual = "@com_github_grpc_grpc//:grpc++_codegen_proto", | |||
| ) | |||
|
|
|||
| native.bind( | |||
There was a problem hiding this comment.
Going forward, please do not use bind if at all possible.
There was a problem hiding this comment.
I am not sure if this is possible, I made a change which won't build. From what I understand, bind is basically aliasing. I removed it and referred to the actual string in the BUILD file. But perhaps you do preprocessing on the BUILD/bzl files to remove the @xyz strings? Is there some config for that, which I also need to update? Is it only pre processing the bzl, and not BUILD files? Just a guess. What do you recommend here?
ajamato@ajamato-linux0:~/grpc$ bazel test :all
INFO: Running bazel wrapper (see //tools/bazel for details), bazel version 1.0.0 will be used instead of system-wide bazel installation.
INFO: Writing tracer profile to '/usr/local/google/home/ajamato/.cache/bazel/_bazel_ajamato/7d9cb851a3ed2d28aac44248d4b651f6/command.profile.gz'
DEBUG: /usr/local/google/home/ajamato/.cache/bazel/_bazel_ajamato/7d9cb851a3ed2d28aac44248d4b651f6/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:226:13: rbe_msan not using checked in configs; Bazel version 1.0.0 was picked/selected with '["9.0.0", "10.0.0"]' compatible configs but none match the 'env = {"ABI_LIBC_VERSION": "glibc_2.19", "ABI_VERSION": "clang", "BAZEL_COMPILER": "clang", "BAZEL_HOST_SYSTEM": "i686-unknown-linux-gnu", "BAZEL_TARGET_CPU": "k8", "BAZEL_TARGET_LIBC": "glibc_2.19", "BAZEL_TARGET_SYSTEM": "x86_64-unknown-linux-gnu", "CC": "clang", "CC_TOOLCHAIN_NAME": "linux_gnu_x86", "BAZEL_LINKOPTS": "-lc++:-lc++abi:-lm"}', 'config_repos = None',and/or 'create_cc_configs = True' passed as attrs
ERROR: /usr/local/google/home/ajamato/grpc/BUILD:2276:1: //:grpc_opencensus_plugin: invalid label '//external:@io_opencensus_cpp//opencensus/trace:context_util' in element 5 of attribute 'deps' in 'cc_library' rule: invalid target name '@io_opencensus_cpp//opencensus/trace:context_util': target names may not contain '//' path separators
ERROR: error loading package '': Package '' contains errors
INFO: Elapsed time: 0.138s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
FAILED: Build did NOT complete successfully (0 packages loaded)
There was a problem hiding this comment.
I see. This appears to be a failure of grpc_cc_library's implementation. It will take a bit more effort to fix than is reasonable to expect from this PR. The binds will be fine for now.
ajamato
force-pushed
the
grpc_census_tags
branch
from
f0946b1
to
e580039
Compare
gnossen
added
lang/c++
release notes: no
gnossen
changed the title
|
Added a test case, PTAL. |
|
The checks don't seem to be completing. Am I supposed to run a manual step for these? I didn't see this on the contribution guide, so I was unsure. Or is it stuck? Can I rerun somehow? And/Or look at a log to debug somehow? |
|
+yashykt for review of the filters. Please reassign if you think there's someone more appropriate to perform the review. |
| @@ -152,6 +158,11 @@ void CensusClientCallData::Destroy(grpc_call_element* elem, | |||
| const uint64_t request_size = GetOutgoingDataSize(final_info); | |||
| const uint64_t response_size = GetIncomingDataSize(final_info); | |||
| double latency_ms = absl::ToDoubleMilliseconds(absl::Now() - start_time_); | |||
| std::vector<std::pair<opencensus::tags::TagKey, std::string>> tags = | |||
| context_.tags().tags(); | |||
| tags.emplace_back(ClientMethodTagKey(), method_); | |||
There was a problem hiding this comment.
Does this need to be std::string(method_) to make this pass?
There was a problem hiding this comment.
I am unsure how to run this test locally, it passes when I run it manually. So I expect its related to a different environment at compilation time. I tried the --use_docker flag as well, but wasn't able to reproduce the failure. Any suggestions?
tools/run_tests/run_tests.py -l c++ --use_docker
I've checked in a commit which compiles, using your std::string(method_) suggestion. Hopefully this will work. Though I am not sure how to trigger the tests to run again
|
Am I doing something wrong here? I am not sure how to trigger these tests to run. Are they just waiting to run in a lengthy test queue? I am not too familiar with how this test queue works. Am I supposed to run a manual step? |
|
@mhaidrygoog Can you please review the C++ portions of this PR? |
ajamato
force-pushed
the
grpc_census_tags
branch
from
c7e7b50
to
2e958cf
Compare
|
"opencensus-trace-context_util" dependency is internal builds. The issue seems to be that this doesn't exist internally and there needs to be a way to bring it in before this PR can be pulled in. Reverting for now. |
Census client filter: use current span and tags.
Add support to use current span and tags, to support census tagging.
original_author was @g-easy, who asked mt to produce a PR from this commit
g-easy@ae02d17
@sheenaqotj