feat: add opentelemetry-exporter-http-transport package#5194
Conversation
|
|
||
| Core package (no HTTP backend included):: | ||
|
|
||
| pip install opentelemetry-exporter-http-transport |
There was a problem hiding this comment.
If this is specific to otlp over http, should the package name reflect that: opentelemetry-exporter-otlp-http-transport
There was a problem hiding this comment.
I was actually debating this exact naming decision, I decided that opentelemetry-exporter-http-transport might be slightly more appropriate than opentelemetry-exporter-otlp-http-transport given that we might want to use the transport abstraction for non-OTLP exporters (e.g. Jaeger). That being said, I still opted to include the OTLPHTTPClient inside this package since it's pretty lean and will be reused by the Proto + JSON HTTP exporters, but we could even break this off into another package, but my thought is this would be a bit overkill. WDYT?
There was a problem hiding this comment.
i think either is ok really. If the plan is to use this for exporters other than otlp, then i agree it makes more sense to keep it this way
There was a problem hiding this comment.
Shouldn't this be named opentelemetry-exporter-otlp-http-common for consistency with the rest of the packages?
There was a problem hiding this comment.
@ocelotl If we were to use this for non-OTLP exporter implementations (e.g. Prometheus remote-write or Zipkin), then it would make more sense to omit the otlp name. However, I'm also open to keeping it in - this was just my original thought process.
There was a problem hiding this comment.
I think generic named packages are the right choice. The package name says "HTTP transport" so the contents should match.
I've vote for opentelemetry-exporter-http-transport as the name and the scope, but move OTLPHTTPClient out. It belongs alongside the OTLP exporters (either in opentelemetry-exporter-otlp-proto-http itself, or in a new opentelemetry-exporter-otlp-http-common if we want to share retry/compression logic across OTLP HTTP variants like JSON).
That leaves this package shipping only BaseHTTPTransport + the requests/urllib3 implementations, which would be reusable with other exporters (Zipkin, Prometheus, etc) and any future HTTP exporter can depend on it without pulling OTLP-specific code. The OTLP retry/backoff/compression logic is small enough to live with its consumer.
We need to get these packages right now or will be a pain to navigate later.
There was a problem hiding this comment.
Agree with @MikeGoldsmith, I will remove the OTLPHTTPClient from this PR and submit a follow-up for opentelemetry-exporter-otlp-http-common.
|
Thanks for the PR! Just a heads-up: we no longer update Please add the appropriate changelog fragment for this change instead of editing |
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Is there already a branch somewhere showing how this package can be used inside some of the other http exporters?
@codeboten I've opened #5219 which includes the full changes updating the OTLP Proto HTTP exporter to use this new package. I've also switched to start using |
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
I'm in favour of this change, it makes a lot of sense to have a shared package.
I've left some comments here, and will add to the on-going conversation around naming.
| return False | ||
|
|
||
|
|
||
| class Compression(enum.Enum): |
There was a problem hiding this comment.
We'll have two versions of this enum, one here and another in opentelemetry.exporter.otlp.proto.http.Compression.
There was a problem hiding this comment.
Open to alternative suggestions here, but I wanted to have an enum here for configuring the compression for the OTLPHTTPClient. We could remove the existing enum in the proto exporter but this would be a breaking change - we'd want to leave it if we want to maintain backwards compatibility
There was a problem hiding this comment.
Once we integrate this package into other exporters we can deprecate the old enum and allow users to pass in both the new and old enum variants.
herin049
requested review from
MikeGoldsmith,
Copilot and
ocelotl
and removed request for
ocelotl
There was a problem hiding this comment.
Pull request overview
Adds a new opentelemetry-exporter-http-transport package intended to provide shared HTTP transport abstractions for exporter implementations, including optional requests and urllib3 backends.
Changes:
- Introduces base HTTP result/transport interfaces and concrete
requests/urllib3transport implementations. - Adds package metadata, README, changelog, lockfile/workspace wiring, tox environments, and CI jobs.
- Adds unit tests and test requirements for both HTTP backend implementations.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.changelog/5194.added |
Adds changelog fragment for the new package. |
.github/workflows/lint.yml |
Adds lint CI job for the new package. |
.github/workflows/test.yml |
Adds test CI jobs for the new package across Python/platform/version matrices. |
dev-requirements.txt |
Adds mocket for test/lint dependency resolution. |
eachdist.ini |
Registers the new package in prerelease distribution tooling. |
exporter/opentelemetry-exporter-http-transport/LICENSE |
Adds package license file. |
exporter/opentelemetry-exporter-http-transport/README.rst |
Documents installation and purpose of the new package. |
exporter/opentelemetry-exporter-http-transport/pyproject.toml |
Defines package metadata, extras, and build configuration. |
exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/__init__.py |
Adds package namespace module. |
exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/_base.py |
Adds abstract HTTP transport/result interfaces. |
exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/_requests.py |
Adds requests-based HTTP transport/result implementation. |
exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/_urllib3.py |
Adds urllib3-based HTTP transport/result implementation. |
exporter/opentelemetry-exporter-http-transport/src/opentelemetry/exporter/http/transport/version/__init__.py |
Adds package version module. |
exporter/opentelemetry-exporter-http-transport/test-requirements.in |
Defines test dependency inputs. |
exporter/opentelemetry-exporter-http-transport/test-requirements.latest.txt |
Adds compiled latest test dependencies. |
exporter/opentelemetry-exporter-http-transport/test-requirements.oldest.txt |
Adds compiled oldest test dependencies. |
exporter/opentelemetry-exporter-http-transport/tests/__init__.py |
Adds test package marker. |
exporter/opentelemetry-exporter-http-transport/tests/test_requests_transport.py |
Adds tests for the requests transport. |
exporter/opentelemetry-exporter-http-transport/tests/test_urllib3_transport.py |
Adds tests for the urllib3 transport. |
pyproject.toml |
Adds workspace and pyright wiring for the new package. |
tox.ini |
Adds tox test/lint environments and pyright dependency wiring. |
uv.lock |
Adds lockfile entries for the new workspace package and extras. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Adds a common
opentelemetry-exporter-http-transportpackage that will be utilized by multiple HTTP exporter packages (initially OTLP JSON) to abstract away and generalize underlying HTTP transport implementations and exporting with OTLP. Introducing this package will significantly reduce the amount of code duplication present in HTTP exporters and will allow for the usage of alternative HTTP client libraries.Fixes #3439, #4171, #1003, #2990
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: