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] Implement OpenTelemetry #35292

Closed
wants to merge 8 commits into from
Closed

[python O11Y] Implement OpenTelemetry #35292

wants to merge 8 commits into from

Conversation

XuanWang-Amos
Copy link
Contributor

No description provided.

@XuanWang-Amos XuanWang-Amos added the release notes: no Indicates if PR should not be in release notes label Dec 12, 2023
@XuanWang-Amos XuanWang-Amos added release notes: yes Indicates if PR needs to be in release notes and removed lang/Python release notes: no Indicates if PR should not be in release notes labels Dec 12, 2023
@eugeneo eugeneo assigned gnossen and unassigned eugeneo Dec 12, 2023
@eugeneo eugeneo requested a review from gnossen December 12, 2023 21:01
@XuanWang-Amos XuanWang-Amos removed the request for review from gnossen December 12, 2023 21:38
@XuanWang-Amos XuanWang-Amos marked this pull request as ready for review December 14, 2023 00:27
gnossen
gnossen reviewed Dec 19, 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. Not a full review yet.

src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/BUILD.bazel Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/__init__.py Show resolved Hide resolved
src/python/grpcio_tests/tests/observability/_open_telemetry_observability_test.py Outdated Show resolved Hide resolved
src/python/grpcio_tests/tests/observability/_open_telemetry_observability_test.py Outdated Show resolved Hide resolved
src/python/grpcio_tests/tests/observability/_open_telemetry_observability_test.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/server_call_tracer.cc Outdated Show resolved Hide resolved
gnossen
gnossen reviewed Dec 20, 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.

I think I've reviewed everything now. Mostly looks good.

src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Outdated
GRPC_OTHER_LABEL_VALUE = "other"


class LabelInjector(abc.ABC):
Copy link
Contributor

@gnossen gnossen Dec 20, 2023 •
edited

Choose a reason for hiding this comment

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

Is this a public interface? I think so because it's a part of the OpenTelemetryPluginOption interface and OpenTelemetryPluginOption is part of the OpenTelemetryPlugin interface, which is definitely public. If so, we'll want to make sure that it's named in a way that indicates that is.

Currently, this is named grpc_observability._open_telemetry_plugin.LabelInjector. The _open_telemetry_plugin part implies that this is private. There are a couple of ways to address this:

  1. You could reexport this in __init__py as grpc_observability.LabelInjector. However, not it's not clear that this is OpenTelemetry-specific, so you might want to rename to grpc_observability.OpenTelemetryLabelInjector.
  2. Create a submodule called opentelemetry, so that this is grpc_observability.opentelemetry.LabelInjector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is a public interface. Since this LabelInjector is only used for OpenTelemetry, I'll rename it to OpenTelemetryLabelInjector.

This is part of CSM observability, after reviewing the design, I found something that needs to be clarified. I'll reexport this once started working on CSM observability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please assign an issue against yourself to create a test similar to this one for the grpc_observability package so that we can protect against regressions in the public API.

src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Outdated Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Show resolved Hide resolved
src/python/grpcio_observability/grpc_observability/_open_telemetry_plugin.py Show resolved Hide resolved
@copybara-service copybara-service bot closed this in d2da19e Jan 2, 2024
XuanWang-Amos added a commit to XuanWang-Amos/grpc that referenced this pull request Jan 3, 2024
@XuanWang-Amos
Revert "[python O11Y] Implement OpenTelemetry (grpc#35292)"
eaad048 
This reverts commit d2da19e.
This was referenced Jan 3, 2024
Closed
Open
Closed
copybara-service bot pushed a commit that referenced this pull request Jan 4, 2024
@XuanWang-Amos
[Python Otel] Re-apply Otel (#35439)
48cf940 
This reverts commit 96b9e8d.

[Implement OpenTelemetry PR](#35292) was [reverted](96b9e8d) because some tests started failing after import the changes to g3.

After investigation, we found root cause, it can be fixed both on our side and on gapic API side, we opened an issue to [gapic API team](googleapis/python-api-core#579), this PR will includes the fixes on our side.

<!--

If you know who should review your pull request, please assign it to that
person, otherwise the pull request would get assigned randomly.

If your pull request is for a specific language, please add the appropriate
lang label.

-->

Closes #35439

COPYBARA_INTEGRATE_REVIEW=#35439 from XuanWang-Amos:reapply_otel 0133564
PiperOrigin-RevId: 595746222
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 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
3 participants
@XuanWang-Amos @gnossen @eugeneo