Add a timeout param to all OTLP grpc / http export calls #4560
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
) * added examples * Apply suggestions from code review Co-authored-by: Emídio Neto <[email protected]> * feat: added examples for metrics and logs * fixed spelling * Update docs/examples/metrics/reader/README.rst Co-authored-by: Emídio Neto <[email protected]> --------- Co-authored-by: Emídio Neto <[email protected]>
* trying sha-automation Signed-off-by: emdneto <[email protected]> * fix label names * fix sha-automation core Signed-off-by: emdneto <[email protected]> * add new line Signed-off-by: emdneto <[email protected]> --------- Signed-off-by: emdneto <[email protected]>
xrmx
reviewed
Apr 29, 2025
| @@ -2,6 +2,7 @@ asgiref==3.7.2 | |||
| Deprecated==1.2.14 | |||
| googleapis-common-protos==1.63.2 | |||
| grpcio==1.66.2 | |||
| grpcio-status==1.66.0 | |||
There was a problem hiding this comment.
Should this be added as dev depdency to the package?
There was a problem hiding this comment.
It's only used for the tests, so I don't think so ?
xrmx
reviewed
Apr 29, 2025
|
|
||
| # pylint: disable=invalid-name,unused-argument | ||
| def Export(self, request, context): | ||
| logger.warning("Export Request Recieved") |
There was a problem hiding this comment.
I actually use this in the tests to figure out how many times export has been called.. If you know of a better way to check that let me know
| def export(self, batch: Sequence[LogData]) -> LogExportResult: | ||
| return self._export(batch) | ||
| def export( | ||
| self, batch: Sequence[LogData], timeout_millis: Optional[float] = None |
There was a problem hiding this comment.
Shouldn't timeout_millis be Optional[int]?
There was a problem hiding this comment.
I don't feel too strongly about it, I think float or int both are fine.. I switched it to int though
Bumps [jinja2](https://github.com/pallets/jinja) from 3.1.5 to 3.1.6. - [Release notes](https://github.com/pallets/jinja/releases) - [Changelog](https://github.com/pallets/jinja/blob/main/CHANGES.rst) - [Commits](pallets/jinja@3.1.5...3.1.6) --- updated-dependencies: - dependency-name: jinja2 dependency-version: 3.1.6 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Riccardo Magliocchetti <[email protected]>
* logs: introduce LogAttributes type Logs attribute accepts AnyValue as AttributeValue add a type to describe that and start using it. * LogAttributes -> ExtendedAttributes * Handle ExtendedAttributes in BoundedAttributes * opentelemetry-sdk: serialize extended attributes * Add changelog * Fix typing * Fix handling of not attribute values inside sequences * Please mypy * Please lint * More typing * Even more typing fixes * Fix docs * Fix mypy * Update LogRecord attributes typing to match reality * More typing * Move changelog to unreleased * ExtendedAttributes -> _ExtendedAttributes * opentelemetry-sdk: keep instrumentation scope attributes as Attributes * exporter/otlp: allow export of none values in logs attributes
) * added examples * Apply suggestions from code review Co-authored-by: Emídio Neto <[email protected]> * feat: added examples for metrics and logs * fixed spelling * Update docs/examples/metrics/reader/README.rst Co-authored-by: Emídio Neto <[email protected]> --------- Co-authored-by: Emídio Neto <[email protected]>
* trying sha-automation Signed-off-by: emdneto <[email protected]> * fix label names * fix sha-automation core Signed-off-by: emdneto <[email protected]> * add new line Signed-off-by: emdneto <[email protected]> --------- Signed-off-by: emdneto <[email protected]>
|
I have bad merge conflicts on this PR, not sure what I did exactly but the diff is really messed up. Moving this over to #4564 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Update
exporton OTLP / HTTP exporters to include a timeout param. Maketimeoutencompass retries, rather than being applied per-request.Update the grpc exporter to us the
retryconfig, which letsgrpchandle retries / timeout automatically based on our config, so we can delete a lot of code.There is no equivalent for HTTP (there is a
urlilib3.RetryConfig, but it's retries are not inclusive oftimeout- instead each retry gets passed the same timeout), so I manually updated the HTTP exporters.I also cleaned up the HTTP exporter code a tiny bit.
#4183 -- similar to this PR and what's discussed in #4043, but I implemented it in as minimal a way as I could..
I updated the
LogExporterandSpanExporterinterfaces to includetimeout_millis(MetricExporteralready has it), this isn't a breaking change because@abstractmethodjust checks that classes implementin have a method with a particular name, it doesn't check method parameters at all.. It does cause a pylint error, but I think that's a good thing.In future PRs I plan to update
shutdownandforceFlushto pass their timeouts along to export.. We could potentially pass these env var timeouts along to all otherexportcalls too..Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Lots of unit tests.
Does This PR Require a Contrib Repo Change?
Checklist: