OTel-Arrow receiver admission limits: redesign #36033

jmacd · 2024-10-28T23:31:19Z

Description

Redesigns the internal/otelarrow/admission package. There was a defect & regression. This follows the solution outlined in #36074.

OTel-Arrow receiver:

Stop using otlp-pdata-size header.
Acquire resources once per request, not in two steps.
admission::waiter_limit configuration (measured in requests) was ineffective, replace with admission::waiting_limit_mib measured in bytes.

Admission controller:

Fix resource leak in context cancellation
Avoid use of fallible APIs
Use LIFO order

Link to tracking issue

Fixes #36074.

Testing

New end-to-end test added. Explicitly test for proper span production when OTLP admission control actions fail.

Documentation

Updated.

codecov · 2024-10-29T02:37:55Z

Codecov Report

Attention: Patch coverage is 99.32432% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.37%. Comparing base (c5b6746) to head (e9b115a).
Report is 57 commits behind head on main.

Files with missing lines	Patch %	Lines
receiver/otelarrowreceiver/otelarrow.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #36033      +/-   ##
==========================================
+ Coverage   80.34%   80.37%   +0.02%     
==========================================
  Files        2138     2142       +4     
  Lines      201222   201669     +447     
==========================================
+ Hits       161673   162090     +417     
- Misses      34274    34304      +30     
  Partials     5275     5275

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…tor-contrib into jmacd/newq

jmacd · 2024-10-29T18:44:35Z

internal/otelarrow/admission/boundedqueue.go

+			// We were also admitted, which can happen
+			// concurrently with cancellation. Make sure
+			// to release since no one else will do it.
+			bq.releaseLocked(pending)


Review notes: this is a new code path, fixes the race condition.

jmacd · 2024-10-29T18:44:54Z

internal/otelarrow/admission/boundedqueue.go

+	case <-waiter.notify.Chan():
+		parentSpan.AddEvent("admission accepted (slow path)", pendingAttr)
+		return bq.releaseFunc(pending), nil
+


Review notes: This is the same logic as before, but we return a release function to avoid fallible interfaces.

jmacd · 2024-10-29T18:45:48Z

internal/otelarrow/admission/boundedqueue.go

-	bq.lock.Unlock()
-	ctx, span := bq.tracer.Start(ctx, "admission_blocked",
-		trace.WithAttributes(attribute.Int64("pending", pendingBytes)))
+	ctx, span := bq.tracer.Start(ctx, "admission_blocked", pendingAttr)


Note: the use of list.List avoids the need to generate a new key. This simplification is possible because we always remove in LIFO order and don't require random access.

jmacd · 2024-10-29T19:16:26Z

receiver/otelarrowreceiver/internal/logs/otlp.go

 	sizer        *plog.ProtoMarshaler
 	logger       *zap.Logger
 }

 // New creates a new Receiver reference.
-func New(logger *zap.Logger, nextConsumer consumer.Logs, obsrecv *receiverhelper.ObsReport, bq *admission.BoundedQueue) *Receiver {
+func New(logger *zap.Logger, nextConsumer consumer.Logs, obsrecv *receiverhelper.ObsReport, bq admission.Queue) *Receiver {


Note there are identical changes in each of the otlp.go and otlp_test.go files below, once each for traces, logs, and metrics.

jmacd · 2024-10-29T19:18:03Z

internal/otelarrow/admission/boundedqueue_test.go

@@ -5,209 +5,330 @@ package admission

 import (
 	"context"
+	"fmt"


Note that this file has been rewritten from scratch because of the FIFO->LIFO change and the fallibility change. This file brings us to 100% test coverage.

#### Description Adds a no-op implementation of the BoundedQueue used by the OTel-Arrow receiver for admission control. #### Link to tracking issue Part of #36074. #### Testing Adds a new end-to-end test to verify admission control with/without waiters and unbounded. The test was taken from #36033. #### Documentation Added: "0" request_limit_mib indicates no admission control.

#### Description Simplifies the admission control logic for OTAP payloads. We call Acquire() once after uncompressing the data, instead of once with compressed size and once with the difference. #### Link to tracking issue Part of #36074. #### Testing One test is replaced with logic to verify certain BoundedQueue actions. ~Note: the OTel-Arrow test suite will not pass with this PR until it merges with #36078.~ Originally developed in #36033. #### Documentation Not user-visible.

…try#36081) #### Description Adds a no-op implementation of the BoundedQueue used by the OTel-Arrow receiver for admission control. #### Link to tracking issue Part of open-telemetry#36074. #### Testing Adds a new end-to-end test to verify admission control with/without waiters and unbounded. The test was taken from open-telemetry#36033. #### Documentation Added: "0" request_limit_mib indicates no admission control.

…telemetry#36082) #### Description Simplifies the admission control logic for OTAP payloads. We call Acquire() once after uncompressing the data, instead of once with compressed size and once with the difference. #### Link to tracking issue Part of open-telemetry#36074. #### Testing One test is replaced with logic to verify certain BoundedQueue actions. ~Note: the OTel-Arrow test suite will not pass with this PR until it merges with open-telemetry#36078.~ Originally developed in open-telemetry#36033. #### Documentation Not user-visible.

…try#36081) #### Description Adds a no-op implementation of the BoundedQueue used by the OTel-Arrow receiver for admission control. #### Link to tracking issue Part of open-telemetry#36074. #### Testing Adds a new end-to-end test to verify admission control with/without waiters and unbounded. The test was taken from open-telemetry#36033. #### Documentation Added: "0" request_limit_mib indicates no admission control.

…telemetry#36082) #### Description Simplifies the admission control logic for OTAP payloads. We call Acquire() once after uncompressing the data, instead of once with compressed size and once with the difference. #### Link to tracking issue Part of open-telemetry#36074. #### Testing One test is replaced with logic to verify certain BoundedQueue actions. ~Note: the OTel-Arrow test suite will not pass with this PR until it merges with open-telemetry#36078.~ Originally developed in open-telemetry#36033. #### Documentation Not user-visible.

jmacd added 5 commits October 28, 2024 11:02

Replace bounded queue w/ LIFO

876e7bd

trace fix

498493c

fix metrics

7ea2e32

fix logs

845b138

New e2e test; use uint64 for consistency w/ config

e9b115a

github-actions bot added internal/otelarrow receiver/otelarrow labels Oct 28, 2024

github-actions bot requested a review from moh-osman3 October 28, 2024 23:31

jmacd added 5 commits October 29, 2024 09:49

Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…

6ef2567

…tor-contrib into jmacd/newq

lint

746dd66

document

e2be3c8

issue number

defc472

goporto

9d32b6c

jmacd commented Oct 29, 2024

View reviewed changes

jmacd added 2 commits October 29, 2024 11:53

remove test

0549d16

new file

7b0af8a

jmacd commented Oct 29, 2024

View reviewed changes

jmacd closed this Oct 29, 2024

This was referenced Oct 29, 2024

(otelarrowreceiver): Support disabling admission control #36081

Merged

(otelarrowreceiver): Use a single call to BoundedQueue.Acquire #36082

Merged

jmacd mentioned this pull request Oct 31, 2024

(otelarrowreceiver) Deprecate admission::waiter_limit #36100

Closed

This was referenced Nov 1, 2024

(internal/otelarrow) Fix an e2e test flake #36138

Closed

(internal/otelarrow) Add new LIFO boundedqueue #36140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTel-Arrow receiver admission limits: redesign #36033

OTel-Arrow receiver admission limits: redesign #36033

jmacd commented Oct 28, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading

jmacd Oct 29, 2024

jmacd Oct 29, 2024

jmacd Oct 29, 2024

jmacd Oct 29, 2024

jmacd Oct 29, 2024

OTel-Arrow receiver admission limits: redesign #36033

OTel-Arrow receiver admission limits: redesign #36033

Conversation

jmacd commented Oct 28, 2024 • edited Loading

Description

Link to tracking issue

Testing

Documentation

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

jmacd Oct 29, 2024

Choose a reason for hiding this comment

jmacd Oct 29, 2024

Choose a reason for hiding this comment

jmacd Oct 29, 2024

Choose a reason for hiding this comment

jmacd Oct 29, 2024

Choose a reason for hiding this comment

jmacd Oct 29, 2024

Choose a reason for hiding this comment

jmacd commented Oct 28, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading