fix(config): allow deflate for OTLP HTTP exporters#5075
fix(config): allow deflate for OTLP HTTP exporters#5075officialasishkumar wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80ca78e192
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if value.lower() == "deflate" and hasattr(compression_enum, "Deflate"): | ||
| return compression_enum.Deflate # type: ignore[attr-defined] |
There was a problem hiding this comment.
The shared mapper now returns Deflate for any enum that exposes that member, and both gRPC exporter builders (_create_otlp_grpc_span_exporter and _create_otlp_grpc_metric_exporter) call this helper, so declarative otlp_grpc.compression: deflate is now accepted. That changes prior validation behavior and creates a mismatch with the gRPC exporter’s own env parsing (environ_to_compression only accepts gzip/none), so users can get different accepted values depending on configuration path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in 053503a by making deflate opt-in for the HTTP exporter call sites while the shared mapper still rejects it by default.
officialasishkumar
force-pushed
the
config-http-deflate
branch
from
80ca78e to
053503a
Compare
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
Generally looks good, thanks @officialasishkumar.
I notice we haven't added deflate support for logs, in the logger provider - was that intentional? Could we add it too?
|
|
||
| - `opentelemetry-sdk`: Add `service` resource detector support to declarative file configuration via `detection_development.detectors[].service` | ||
| ([#5003](https://github.com/open-telemetry/opentelemetry-python/pull/5003)) | ||
| - `opentelemetry-sdk`: Allow declarative OTLP HTTP exporters to map `compression: deflate` instead of rejecting it as unsupported |
There was a problem hiding this comment.
| - `opentelemetry-sdk`: Allow declarative OTLP HTTP exporters to map `compression: deflate` instead of rejecting it as unsupported | |
| - `opentelemetry-sdk`: Allow declarative OTLP HTTP exporters to map `compression: deflate` instead of rejecting it as unsupported | |
| ([#5003](https://github.com/open-telemetry/opentelemetry-python/pull/5075)) |
| if ( | ||
| value.lower() == "deflate" | ||
| and allow_deflate | ||
| and hasattr(compression_enum, "Deflate") |
There was a problem hiding this comment.
I think we should separate these checks. If the user sets allow_deflat=True but the compression_enum doesn't include Deflate, we return an error to indicate what compression types are available (eg "Supported values: 'gzip', 'none'").
With them separate, we can check if Deflate is avaiable and return an error with that instead.
| "Install it with: pip install opentelemetry-exporter-otlp-proto-grpc" | ||
| ) from exc | ||
|
|
||
| compression = _map_compression(config.compression, grpc.Compression) |
There was a problem hiding this comment.
I notice we're not setting allow_deflate=True for gprc - is that intentional?
github-project-automation
bot
moved this to Reviewed PRs that need fixes
in Python PR digest
|
|
||
|
|
||
| def _map_compression( | ||
| value: Optional[str], |
There was a problem hiding this comment.
nit:
| value: Optional[str], | |
| value: str | None, |
(and elsewhere in this PR)
| ): | ||
| return compression_enum.Deflate # type: ignore[attr-defined] | ||
|
|
||
| supported_values = ["'gzip'", "'none'"] |
There was a problem hiding this comment.
We can probably move this code to up towards the beginning of the function and update lines 66-68 to just check if value.lower() in supported_values.
Map declarative OTLP compression values through a shared helper that recognizes Deflate when the exporter enum supports it, while leaving gRPC validation unchanged. Add regression coverage for the shared mapping helper plus tracer and meter provider HTTP exporter construction.
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
officialasishkumar
force-pushed
the
config-http-deflate
branch
from
053503a to
63ad286
Compare
|
Extended the declarative OTLP HTTP deflate handling to the logs path as well, updated the shared helper, and force-pushed the branch. |
3 participants
Description
Fixes #5059
Map declarative OTLP compression values through a shared helper that recognizes
Deflatewhen the target exporter enum supports it. This allows the tracer and meter OTLP HTTP exporters to acceptcompression: deflatewithout changing gRPC validation.Type of change
How Has This Been Tested?
/tmp/opentelemetry-python/.venv-codex/bin/python -m pytest opentelemetry-sdk/tests/_configuration/test_common.py opentelemetry-sdk/tests/_configuration/test_tracer_provider.py opentelemetry-sdk/tests/_configuration/test_meter_provider.py/tmp/opentelemetry-python/.venv-codex/bin/python -m ruff check opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_common.py opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_tracer_provider.py opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_meter_provider.py opentelemetry-sdk/tests/_configuration/test_common.py opentelemetry-sdk/tests/_configuration/test_tracer_provider.py opentelemetry-sdk/tests/_configuration/test_meter_provider.pyDoes This PR Require a Contrib Repo Change?
Checklist: