[WIP] gRPC instrumentation migration to release candidate semconv (1.40.0)#4279
[WIP] gRPC instrumentation migration to release candidate semconv (1.40.0)#4279lmolkova wants to merge 4 commits intoopen-telemetry:mainfrom
Conversation
| UV_CONFIG_FILE={toxinidir}/tox-uv.toml | ||
| PIP_CONSTRAINTS={toxinidir}/test-constraints.txt | ||
| UV_BUILD_CONSTRAINT={toxinidir}/test-constraints.txt | ||
| grpc: PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python |
| if _report_new(self._sem_conv_opt_in_mode): | ||
| async def _unimplemented(_request, context): | ||
| self._handle_unimplemented(handler_call_details, context) | ||
| # TODO: I still don't like it, figure out how not to |
| tracer_provider = kwargs.get("tracer_provider") | ||
| request_hook = self._request_hook | ||
| response_hook = self._response_hook | ||
| target = args[0] if args else kwargs.get("target", "") |
There was a problem hiding this comment.
todo: are there other ways to pass target /host:port to public api?
| if mode is _StabilityMode.DEFAULT: | ||
| return "https://opentelemetry.io/schemas/1.11.0" | ||
| # TODO: update to 1.40.0 | ||
| return Schemas.V1_38_0.value |
| if _report_new(sem_conv_opt_in_mode): | ||
| span.set_attribute(ERROR_TYPE, type(exc).__qualname__) | ||
|
|
||
| if _report_old(sem_conv_opt_in_mode): |
There was a problem hiding this comment.
TODO: report log-based event on the new code path
|
@tammy-baylis-swi I believe you've beed doing quite a few of these conversions for HTTP and DB (🫶), would you mind taking a look if I'm following common patterns on stability opt in here? |
| if is_error: | ||
| if _report_new(sem_conv_opt_in_mode): | ||
| span.set_attribute(ERROR_TYPE, code.name if code else ErrorTypeValues.OTHER) | ||
| status_description = f"{code}:{description}" if description else str(code) |
There was a problem hiding this comment.
this does not look right - no need to duplicate code in the span status
Hi @lmolkova !! Overall this makes sense to me so far. I'll add some other comments with thoughts. When the PR is closer to being ready, instrumentation/README and the grpc |
| For example, if you assign ``"GRPCTestServer,GRPCHealthServer"`` to the variable, | ||
| then the global interceptor automatically adds the filters to exclude requests to | ||
| services ``GRPCTestServer`` and ``GRPCHealthServer``. | ||
| ``OTEL_SEMCONV_STABILITY_OPT_IN`` |
There was a problem hiding this comment.
Thank you for this 🧡 The readthedocs of each instrumentor were what we're thinking to properly document this, e.g. #4254
| excluded_service_list = [] | ||
| return excluded_service_list | ||
|
|
||
| def _get_rpc_schema_url(mode: _StabilityMode) -> str: |
There was a problem hiding this comment.
I have a similar helper in progress housed somewhere else, but it's mainly for instrumentors that export multiple types e.g. SQLAlchemy doing both HTTP and DATABASE. In distant future and if it makes sense, would be good to have just one.
| # New stable RPC attribute names. Not yet published as stable constants in the | ||
| # opentelemetry-semantic-conventions package because the stable RPC conventions | ||
| # are still a work in progress. | ||
| RPC_SYSTEM_NAME = "rpc.system.name" |
There was a problem hiding this comment.
Just 2 weeks ago we bumped to 1.39.0, so some of these are now available in _incubating.attributes.rpc_attributes
There was a problem hiding this comment.
I think this makes sense as a home for these, unless we wanted to bloat otel.instrumentation._semconv. None of the other instrumentors set rpc span/metrics attributes.
2 participants
This PR is a prototype of gRPC instrumentation following new semconv 1.40.0 https://github.com/open-telemetry/semantic-conventions/blob/v1.40.0/docs/rpc/rpc-spans.md
It uses
OTEL_SEMCONV_STABILITY_OPT_INto opt-into new semconv, supportsrpcandrpc/dup.It does not include metrics - they are not emitted by current instrumentation.
TODOs: