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.

Sign up for GitHub

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

Jump to bottom

[python O11Y] Initial Implementation #32974

Merged
merged 44 commits into from Jun 5, 2023
Merged

[python O11Y] Initial Implementation #32974

merged 44 commits into from Jun 5, 2023

Conversation

XuanWang-Amos
Copy link
Contributor

@XuanWang-Amos XuanWang-Amos commented Apr 29, 2023 •
edited

Testing Command: bazel test --cache_test_results=no --test_output=streamed --runs_per_test=1 --test_timeout=10 "//src/python/grpcio_tests/tests/observability:_observability_test"

TODO:

  • Better error handling.

@XuanWang-Amos XuanWang-Amos added the release notes: yes Indicates if PR needs to be in release notes label Apr 29, 2023
@XuanWang-Amos XuanWang-Amos requested a review from gnossen May 3, 2023 21:43
@XuanWang-Amos XuanWang-Amos marked this pull request as ready for review May 3, 2023 21:43
@gnossen
Copy link
Contributor

gnossen commented May 8, 2023

Looks like there are build issues in CI:

ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: missing input file '//src/python/grpcio_observability/grpc_observability:client_call_tracer.h'
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: missing input file '//src/python/grpcio_observability/grpc_observability:python_census_context.h'
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: missing input file '//src/python/grpcio_observability/grpc_observability:sampler.h'
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: missing input file '//src/python/grpcio_observability/grpc_observability:server_call_tracer.h'
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: 4 input file(s) do not exist
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11 Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: 4 input file(s) do not exist
INFO: Elapsed time: 283.638s, Critical Path: 66.59s
INFO: 3692 processes: 1387 internal, 2305 processwrapper-sandbox.
FAILED: Build did NOT complete successfully

gnossen
gnossen reviewed May 9, 2023
View reviewed changes
Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

Just a first batch. Sorry for the delay.

requirements.bazel.txt Outdated Show resolved Hide resolved
BUILD Outdated Show resolved Hide resolved
src/cpp/ext/gcp/BUILD Outdated Show resolved Hide resolved
src/python/grpcio/grpc/__init__.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
@XuanWang-Amos
Copy link
Contributor Author

Looks like there are build issues in CI:

ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: missing input file '//src/python/grpcio_observability/grpc_observability:client_call_tracer.h'
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: missing input file '//src/python/grpcio_observability/grpc_observability:python_census_context.h'
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: missing input file '//src/python/grpcio_observability/grpc_observability:sampler.h'
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: missing input file '//src/python/grpcio_observability/grpc_observability:server_call_tracer.h'
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11: Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: 4 input file(s) do not exist
ERROR: /var/local/jenkins/grpc/src/python/grpcio_observability/grpc_observability/BUILD.bazel:19:11 Middleman _middlemen/_S_Ssrc_Spython_Sgrpcio_Uobservability_Sgrpc_Uobservability_Cobservability-cc_library-compile failed: 4 input file(s) do not exist
INFO: Elapsed time: 283.638s, Critical Path: 66.59s
INFO: 3692 processes: 1387 internal, 2305 processwrapper-sandbox.
FAILED: Build did NOT complete successfully

I didn't include all files in this PR (the server and client tracer), will probably include them later or in another PR.

@gnossen
Copy link
Contributor

gnossen commented May 9, 2023

or in another PR.

We cannot break CI in this PR, so they'll need to be included in this PR. Generally speaking, any PR put up for review should be complete, in the sense that it is buildable and would cause no issues if merged to the master branch.

This can also cause issues during review if there's a reference to code that is not included in the PR. I see a reference, go looking for it, and can't find it. If the issue is that the client and server tracers are still being worked on or not currently fit for inclusion on the master branch, you can just include a stub for the moment -- a no-op implementation of the interface.

@XuanWang-Amos
Copy link
Contributor Author

or in another PR.

We cannot break CI in this PR, so they'll need to be included in this PR. Generally speaking, any PR put up for review should be complete, in the sense that it is buildable and would cause no issues if merged to the master branch.

This can also cause issues during review if there's a reference to code that is not included in the PR. I see a reference, go looking for it, and can't find it. If the issue is that the client and server tracers are still being worked on or not currently fit for inclusion on the master branch, you can just include a stub for the moment -- a no-op implementation of the interface.

Makes sense. My initial thought was that this PR was already huge, and since the tracer changes are somewhat independent from the rest of the changes, I excluded them for now. I'll add them soon.

@XuanWang-Amos XuanWang-Amos changed the title [Python O11Y] Draft [python O11Y] Draft May 10, 2023
gnossen
gnossen reviewed May 11, 2023
View reviewed changes
Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

Another batch.

src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_server.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/BUILD.bazel Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/BUILD.bazel Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/BUILD.bazel Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/BUILD.bazel Show resolved Hide resolved
gnossen
gnossen reviewed May 11, 2023
View reviewed changes
Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

Another batch.

gnossen
gnossen reviewed May 11, 2023
View reviewed changes
Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

One more batch.

src/python/grpcio_tests/tests/observability/_observability_test.py Outdated Show resolved Hide resolved
src/python/grpcio_tests/tests/observability/_observability_test.py Outdated Show resolved Hide resolved
src/python/grpcio_tests/tests/observability/_observability_test.py Outdated Show resolved Hide resolved
src/python/grpcio_tests/tests/observability/_observability_test.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio/grpc/_observability.py Outdated Show resolved Hide resolved
src/python/grpcio_tests/tests/observability/_observability_test.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/BUILD.bazel Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/observability_main.cc Outdated Show resolved Hide resolved
@XuanWang-Amos
Enable clang forat and iwyu
aa70a52
@XuanWang-Amos
Allow empty config
669ed94
@XuanWang-Amos
Add two more tests
ad8b521
@XuanWang-Amos
Undo accidental changes in C++
b4fc2fb
@XuanWang-Amos
Use random for testConfigFileOverEnvVar
95cf97d
@XuanWang-Amos
Reformat, Use grpc_core::Mutex and use config file in observability t…
137c008 
…ests
@XuanWang-Amos
Fix clang format issue
9984d17
@XuanWang-Amos
Flush one last time before shutdown export thread
fbcb961
@XuanWang-Amos
Manually allocate and free memory for ServerCallTracer
22a26b3
@XuanWang-Amos
Add timeout for exporting thread
15fa364
@XuanWang-Amos
Add some TODOs
c18c9e3
@XuanWang-Amos
Revert format change in __init__.py
f619207
@XuanWang-Amos
More const correctness fixes
57f889d
@XuanWang-Amos
Add OC exporter mock
ef12cc2
@XuanWang-Amos
Triger export based on CensusData count & rasie NotImplementedError f…
089e1cd 
…or OC exporter
@XuanWang-Amos
Fix clang_format_code
4805029
@XuanWang-Amos
Refactoring some classes out of grpc_observability/__init__.py
d5569d0
@XuanWang-Amos
Miscs
969ab8f
@XuanWang-Amos
Fix sanity test
e1ba9c4
@XuanWang-Amos
Fix sanity
e71a35e
@XuanWang-Amos
Misc
bc4a493
@XuanWang-Amos
Enable pytype check for observability package
75f3bef
@XuanWang-Amos
Add docstrings and comments
800a521
@XuanWang-Amos XuanWang-Amos merged commit 629b7a1 into grpc:master Jun 5, 2023
53 of 65 checks passed
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Jun 7, 2023
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Jun 7, 2023
@XuanWang-Amos @eugeneo
[python O11Y] Initial Implementation (grpc#32974)
37f3110 
Testing Command: `bazel test --cache_test_results=no
--test_output=streamed --runs_per_test=1 --test_timeout=10
"//src/python/grpcio_tests/tests/observability:_observability_test"`

### TODO:
 * Better error handling.
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Jun 7, 2023
@XuanWang-Amos @eugeneo
[python O11Y] Initial Implementation (grpc#32974)
4e4842b 
Testing Command: `bazel test --cache_test_results=no
--test_output=streamed --runs_per_test=1 --test_timeout=10
"//src/python/grpcio_tests/tests/observability:_observability_test"`

### TODO:
 * Better error handling.
mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jun 15, 2023
@XuanWang-Amos @mario-vimal
[python O11Y] Initial Implementation (grpc#32974)
1ab7ba0 
Testing Command: `bazel test --cache_test_results=no
--test_output=streamed --runs_per_test=1 --test_timeout=10
"//src/python/grpcio_tests/tests/observability:_observability_test"`

### TODO:
 * Better error handling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnossen gnossen gnossen approved these changes

Assignees

@gnossen gnossen

Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/c++ lang/Python per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@XuanWang-Amos @gnossen @drfloob @veblush @yashykt @grpc-kokoro