feat: Add 'opentelemetry-exporter-otlp-json-http' package#5051
feat: Add 'opentelemetry-exporter-otlp-json-http' package#5051herin049 wants to merge 16 commits intoopen-telemetry:mainfrom
Conversation
herin049
added
the
Approve Public API check
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This is massive at over 9k lines. I'm generally not in favour of such large PRs as you can't approve with confidence.
Saying that, I did notice a couple of things we should look at.
| return MetricExportResult.FAILURE | ||
|
|
||
| export_request = encode_metrics(metrics_data) | ||
| deadline = time.time() + self._timeout |
There was a problem hiding this comment.
timeout_millis is being ignored here — the deadline is always calculated from self._timeout (set at construction time). I think we need to honour the per-call timeout and take the more restrictive of the two, something like:
timeout_secs = min(timeout_millis / 1000, self._timeout) if timeout_millis is not None else self._timeout
deadline = time.time() + timeout_secsNote the unit conversion — timeout_millis is in milliseconds but self._timeout is in seconds.
| "cert_file": client_certificate_file, | ||
| "key_file": client_key_file, | ||
| } | ||
| if certificate is False: |
There was a problem hiding this comment.
Does this work with urllib3? I think that usesca_certs not cert_file. If both certificate and client_certificate_file are provided, mTLS would break right?
There was a problem hiding this comment.
Good catch, line 78 should use ca_certs
github-project-automation
bot
moved this from Ready for review
to Reviewed PRs that need fixes
in Python PR digest
This PR will be dramatically smaller once the PR to add |
2 participants
Description
Follow up to #4996 and finishes the implementation of the OTLP JSON exporter.
Fixes #1003.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Tests have been added to the
opentelemetry-exporter-otlp-json-httpandopentelemetry-exporter-otlp-json-commonpackages, and the existing Docker tests have been completely rewritten to test gRPC, proto-HTTP and JSON-HTTP exporters with all signal types.Does This PR Require a Contrib Repo Change?
Checklist: