Skip to content

test(generator): sandbox verification of spec-driven telemetry compliance tests - #18468

Draft
chalmerlowe wants to merge 93 commits into
mainfrom
sandbox/otel-spec-testing
Draft

chalmerlowe wants to merge 93 commits into
mainfrom
sandbox/otel-spec-testing

Conversation

@chalmerlowe

Copy link
Copy Markdown
Contributor

Description

Temporary sandbox pull request to validate clean-slate spec-driven telemetry compliance tests in GitHub Actions CI without impacting PR #18433.

Scope

  • Replaces legacy boilerplate with table-driven tests parametrized over all 22 feature rows in telemetry_requirements_matrix.csv.
  • All 22 features pass locally in Docker Nox (showcase-3.10).

… hook

- Add rpc.system.name: 'grpc'
- Extract server.address and server.port from client options endpoint
- Extract gcp.grpc.resend_count from request resend count
- Extract gcp.resource.destination.id from request name or parent
- Add _client_response_hook for status code, error.type, and status.message
- Plumb response_hook into get_otel_interceptor and get_otel_async_interceptor
- Test endpoint attribute parsing across host/port variations
- Test destination id and resend count extraction
- Test client request and response hooks covering all status and error cases
- Test interceptor creation and custom endpoint attribute propagation
- Achieve 100% statement and branch coverage on _observability.py
…and hooks

- Rename _extract_t4_attributes to _extract_grpc_request_attributes
- Rename _make_client_request_hook to _make_grpc_client_request_hook
- Rename _client_request_hook to _grpc_client_request_hook
- Rename _client_response_hook to _grpc_client_response_hook
- Preserve generic _extract_endpoint_attributes for shared transport usage
…ntion

- Rename test_extract_t4_attributes to test_extract_grpc_request_attributes
- Rename test_client_request_hook to test_grpc_client_request_hook
- Rename test_client_response_hook to test_grpc_client_response_hook
- Update interceptor hook references to _grpc_client_* hooks
- Add url.domain extraction from universe_domain or default to googleapis.com
- Add _extract_error_attributes helper to extract gcp.errors.domain and gcp.errors.metadata.<key>
- Omit server.port when port matches scheme defaults (443 for https/grpc, 80 for http)
- Remove redundant _grpc_client_response_hook and _STATUS_CODE_NAMES
- Deduplicate name and parent resource lookup for gcp.resource.destination.id
- Add comprehensive parametrized unit tests and update interceptor test suites
…tem attribute

- Strip leading slash from gRPC attempt span names via span.update_name
- Set rpc.method to the fully qualified method name per PRD specification
- Retain rpc.system.name: 'grpc' and remove legacy rpc.system attribute to avoid duplication
- Update unit tests to verify span name normalization and attribute deduplication
- Remove gcp.resource.destination.id extraction from _extract_grpc_request_attributes
- Update unit tests to reflect attribute removal per July Strategy Update
- Broaden transport check in client.py.j2 to allow gRPC transport subclasses.
- Align version comments in client.py.j2 and grpc.py.j2 to 2.36.0+.
- Synchronize all golden client and transport files with template updates.
- Harden zero-overhead and custom tracer provider isolation assertions in test_tracing.py.
- Add direct client initialization test to verify template injection end-to-end.
…port template

- Place ClientInterceptor import under if TYPE_CHECKING: in grpc.py.j2 to eliminate runtime import overhead and avoid import failures on older google-api-core versions.
- String-quote "ClientInterceptor" in the interceptors type annotation for GrpcTransport.__init__.
- Regenerate and synchronize all golden gRPC transport files.
Align if TYPE_CHECKING: in golden gRPC transport files with # pragma: NO COVER to match grpc.py.j2 template output.
…ttp_request

- Eliminate redundant pass-through factory function def trace_http_request.
- Directly implement __enter__ and __exit__ on class trace_http_request.
… branch

- Drop # pragma: NO COVER annotations from lines 181-183 in base.py.j2.
- Remove outdated comment since unit tests explicitly exercise this fallback.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request integrates OpenTelemetry tracing and observability capabilities into the generated Python GAPIC clients across REST and gRPC transports (both synchronous and asynchronous). Key updates include wrapping HTTP requests with tracing context, injecting OpenTelemetry interceptors into gRPC channels, and adding corresponding unit tests. Feedback on the changes highlights a fragility in the newly added async gRPC transport unit tests, where grpc_helpers_async.apply_channel_interceptors is not mocked, potentially causing test failures in environments where this helper is available. Mocking this helper is recommended to ensure test robustness.

Comment on lines +1030 to +1047
def test_{{ service.name|snake_case }}_grpc_asyncio_transport_channel_interceptors():
mock_interceptor = mock.Mock()
mock_channel = mock.Mock()
mock_channel._unary_unary_interceptors = []

with mock.patch.object(
transports.{{ service.grpc_asyncio_transport_name }},
"create_channel",
return_value=mock_channel,
) as mock_create_channel:
transport = transports.{{ service.grpc_asyncio_transport_name }}(
credentials=ga_credentials.AnonymousCredentials(),
interceptors=[mock_interceptor],
)

assert mock_create_channel.call_count == 1
assert mock_interceptor in transport.grpc_channel._unary_unary_interceptors
assert transport.grpc_channel == mock_channel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The test test_{{ service.name|snake_case }}_grpc_asyncio_transport_channel_interceptors does not mock grpc_helpers_async.apply_channel_interceptors. In environments where apply_channel_interceptors is available in google-api-core, it will be called and will return a wrapped InterceptedChannel instance instead of mock_channel. This will cause the assertion assert transport.grpc_channel == mock_channel to fail, making the test fragile.

To make the test robust and consistent with the synchronous gRPC transport tests, we should mock grpc_helpers_async.apply_channel_interceptors to return mock_channel and assert that it was called with the expected interceptors.

def test_{{ service.name|snake_case }}_grpc_asyncio_transport_channel_interceptors():
    mock_interceptor = mock.Mock()
    mock_channel = mock.Mock()

    with (
        mock.patch.object(
            transports.{{ service.grpc_asyncio_transport_name }},
            "create_channel",
            return_value=mock_channel,
        ) as mock_create_channel,
        mock.patch.object(
            grpc_helpers_async,
            "apply_channel_interceptors",
            return_value=mock_channel,
            create=True,
        ) as mock_apply_interceptors,
    ):
        transport = transports.{{ service.grpc_asyncio_transport_name }}(
            credentials=ga_credentials.AnonymousCredentials(),
            interceptors=[mock_interceptor],
        )

        assert mock_create_channel.call_count == 1
        mock_apply_interceptors.assert_called_once_with(
            mock_channel, [mock_interceptor, transport._interceptor]
        )
        assert transport.grpc_channel == mock_channel

Comment on lines +1050 to +1077
def test_{{ service.name|snake_case }}_grpc_asyncio_transport_otel_channel_interceptor():
mock_otel_interceptor = mock.Mock()
mock_obs = mock.Mock()
mock_obs.get_otel_async_interceptor.return_value = mock_otel_interceptor
mock_channel = mock.Mock()
mock_channel._unary_unary_interceptors = []

with (
mock.patch(
"{{ (api.naming.module_namespace + (api.naming.versioned_module_name,) + service.meta.address.subpackage)|join('.') }}.services.{{ service.name|snake_case }}.transports.grpc_asyncio._observability",
mock_obs,
),
mock.patch.object(
transports.{{ service.grpc_asyncio_transport_name }},
"create_channel",
return_value=mock_channel,
) as mock_create_channel,
):
options = client_options.ClientOptions()
transport = transports.{{ service.grpc_asyncio_transport_name }}(
credentials=ga_credentials.AnonymousCredentials(),
client_options=options,
)

mock_obs.get_otel_async_interceptor.assert_called_once_with(options)
assert mock_create_channel.call_count == 1
assert mock_otel_interceptor in transport.grpc_channel._unary_unary_interceptors
assert transport.grpc_channel == mock_channel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The test test_{{ service.name|snake_case }}_grpc_asyncio_transport_otel_channel_interceptor does not mock grpc_helpers_async.apply_channel_interceptors. In environments where apply_channel_interceptors is available in google-api-core, it will be called and will return a wrapped InterceptedChannel instance instead of mock_channel. This will cause the assertion assert transport.grpc_channel == mock_channel to fail, making the test fragile.

To make the test robust and consistent with the synchronous gRPC transport tests, we should mock grpc_helpers_async.apply_channel_interceptors to return mock_channel and assert that it was called with the expected interceptors.

def test_{{ service.name|snake_case }}_grpc_asyncio_transport_otel_channel_interceptor():
    mock_otel_interceptor = mock.Mock()
    mock_obs = mock.Mock()
    mock_obs.get_otel_async_interceptor.return_value = mock_otel_interceptor
    mock_channel = mock.Mock()

    with (
        mock.patch(
            "{{ (api.naming.module_namespace + (api.naming.versioned_module_name,) + service.meta.address.subpackage)|join('.') }}.services.{{ service.name|snake_case }}.transports.grpc_asyncio._observability",
            mock_obs,
        ),
        mock.patch.object(
            transports.{{ service.grpc_asyncio_transport_name }},
            "create_channel",
            return_value=mock_channel,
        ) as mock_create_channel,
        mock.patch.object(
            grpc_helpers_async,
            "apply_channel_interceptors",
            return_value=mock_channel,
            create=True,
        ) as mock_apply_interceptors,
    ):
        options = client_options.ClientOptions()
        transport = transports.{{ service.grpc_asyncio_transport_name }}(
            credentials=ga_credentials.AnonymousCredentials(),
            client_options=options,
        )

        mock_obs.get_otel_async_interceptor.assert_called_once_with(options)
        assert mock_create_channel.call_count == 1
        mock_apply_interceptors.assert_called_once_with(
            mock_channel, [transport._interceptor, mock_otel_interceptor]
        )
        assert transport.grpc_channel == mock_channel

This branch has not been deployed

No deployments
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.

1 participant