Integration testing for memory limits #210

jmacd · 2024-06-04T21:08:00Z

This contains 3 PRs worth of work and will be refactored.
The sequence is as follows:

While race-testing the new integration test, the Arrow receiver turned up a bug identified by the race detector. the inFlightWG was shared by all streams in the receiver, so it would work in testing (almost always) but was incorrect. This may explain unusually long stream shutdowns.
The Arrow exporter was not consistently using gRPC status errors, which the Reciever had fixed in [otel-arrow/receiver] Receiver concurrency fixes; readability improvements & restructuring #205; the code was out-of-line with the design of the top-level directory. Whereas the OTel-Arrow exporter had been inserting consumererror.NewPermanent() wrappers, it is the Exporter module which supports standard OTLP and Arrow one layer up that is responsible for permanent error labeling. Returning gRPC status errors is always preferred to fmt.Errorf in gRPC components.
A new memory-limited integration test is added, which exposed the need for correct status error handling, also exposed an Arrow Consumer bug. The underlying Arrow IPC Reader object has an Err() method that was not being checked--instead we had (3a) a fallback mechanism, checking expected record count, and (3b) write to os.Stderr because of the (known) missing error.

…d/limits

jmacd · 2024-06-04T23:13:03Z

Replaced by all the above mentions.

Part of #210. This updates memory-limit error handling because, prior to this fix, there were two fallbacks. (1) the use of os.Stderr to print the message that we had lost (2) a test for the expected number of records to match The first is now removed (i.e. no printing to os.Stderr). The second is now an internal error. The consumer is expected to test for the memory limit error explicitly (e.g., and handle it as ResourceExhausted). A new function is added to do this (NewLimitErrorFromError) that parses the message using a regexp. An upstream issue report and PR will be filed with Arrow-Go to overcome this in the future. See apache/arrow#41989.

Part of #210. Mainly, changes the use of `fmt.Errorf()` and bare `context.Context.Err()` values, uses gRPC-Go's `status.Errorf()` to wrap the error with a code that gRPC and its consumers recognize. The code was out-of-line with the design of the top-level directory. Whereas the OTel-Arrow exporter had been inserting consumererror.NewPermanent() wrappers, it is the Exporter module which supports standard OTLP and Arrow one layer up that is responsible for permanent error labeling. Returning gRPC status errors is always preferred to fmt.Errorf in gRPC components. Secondly, re-order and rename of the fields passed to the "arrow stream error" log statement, so that it matches the Reciever. This is used as the basis of a test for logging consistency and was otherwise an unintentional disagreement ("which" and "where").

Part of #210. Mainly, fixes a race in the use of `inFlightWG` which is meant to be per-stream but is per-Receiver presently. Secondly, re-order and rename of the fields passed to the "arrow stream error" log statement, so that it matches the Exporter. This is used as the basis of a test for logging consistency and there was otherwise an unintentional disagreement ("which" and "where").

This is consistent with gRPC behavior for unary exporters, as with for HTTP exporters. It also helps us test with realistic conditions in the new integration test of #210. https://grpc.io/docs/guides/deadlines/ explains why the default behavior is the way it is for unary gRPC. This is also the case for Golang's net/http client, which is used for the OTLP/HTTP exporter. Therefore, it seems the right thing to do for Arrow as well. Further, if we were to support extending the deadline on purpose, it would belong in the exporterhelper code (IMO). --------- Co-authored-by: Laurent Quérel <laurent.querel@gmail.com>

The new test ensures that we record memory limit errors Replaces #210. Depends on #211, #212, #213, and particularly #215.

jmacd added 16 commits June 3, 2024 09:59

simple refactor

0c3f03d

test w/ more streams

1a05525

handle reader error

c9d7adc

refactor for reuse

ffe5758

missing release

f0bdffb

arrow v16

2d7c1a2

Error handling in consumer fix -- partial

43efea4

use more status.Errorf

ca38410

test fixes

f6a8a42

revert arrow v16

8fc3c98

revert comment

d40cf41

use status.Error()

30b5d51

comment receiverStream

0fc86a9

Merge branch 'main' of github.com:open-telemetry/otel-arrow into jmac…

55c15e0

…d/limits

revert all gomods

ed3d108

fix test

b5cfa27

jmacd closed this Jun 4, 2024

jmacd added a commit that referenced this pull request Jun 5, 2024

Add a memory limit integration test (#216)

53697cf

The new test ensures that we record memory limit errors Replaces #210. Depends on #211, #212, #213, and particularly #215.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration testing for memory limits #210

Integration testing for memory limits #210

jmacd commented Jun 4, 2024 •

edited

Loading

jmacd commented Jun 4, 2024

Integration testing for memory limits #210

Integration testing for memory limits #210

Conversation

jmacd commented Jun 4, 2024 • edited Loading

jmacd commented Jun 4, 2024

jmacd commented Jun 4, 2024 •

edited

Loading