Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(otelarrowreceiver): add admission control to otlp path #35021

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

moh-osman3
Copy link
Contributor

Description:
This PR helps memory issues in the standard OTLP data path by sharing the semaphore used for admission control in the arrow data path.

@jmacd
Copy link
Contributor

jmacd commented Sep 5, 2024

@moh-osman3 I think it will save us and the users some work if we leave a compatibility mode in place for several releases, allowing the former arrow::admission settings to be used. I would do this by leaving copies of the two old fields in the ArrowConfig struct, with the factory defaulting both fields to zero. If during Validate() the field is set to non-zero, then it overrides the new field.

I think this can be done with

var _ component.ConfigValidator = (*Config)(nil)

func (cfg *Config) Validate() error {
    if err := cfg.GRPC.Validate(); err != nil {
        return err
    }
    if err := cfg.Arrow.Validate(); err != nil {
        return err
    }
    if cfg.Arrow.MemoryLimitMiB != 0 {
        cfg.Admission.MemoryLimitMiB = cfg.Arrow.MemoryLimitMiB
    }
    // ... same for waiters
    return nil
}

@moh-osman3 moh-osman3 marked this pull request as ready for review September 6, 2024 16:10
@moh-osman3 moh-osman3 requested review from a team and jpkrohling September 6, 2024 16:10
receiver/otelarrowreceiver/internal/logs/otlp.go Outdated Show resolved Hide resolved
receiver/otelarrowreceiver/internal/metrics/otlp.go Outdated Show resolved Hide resolved
receiver/otelarrowreceiver/internal/trace/otlp.go Outdated Show resolved Hide resolved
receiver/otelarrowreceiver/testdata/config.yaml Outdated Show resolved Hide resolved
@moh-osman3
Copy link
Contributor Author

Hmm test failing because

Vulnerability #1: GO-2024-3106
    Stack exhaustion in Decoder.Decode in encoding/gob
  More info: https://pkg.go.dev/vuln/GO-2024-3106
  Standard library
    Found in: encoding/gob@go1.22.6
    Fixed in: encoding/gob@go1.22.7
    Example traces found:
Error:       #1: receiver.go:158:45: googlecloudmonitoringreceiver.monitoringReceiver.initializeClient calls google.FindDefaultCredentials, which eventually calls gob.Decoder.Decode

Your code is affected by 1 vulnerability from the Go standard library.
This scan also found 0 vulnerabilities in packages you import and 2
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.

@crobert-1
Copy link
Member

Hmm test failing because

It looks like this CVE was just published today, it will need resolved in its own PR. It's failing on main, and unrelated to this change. 👍

receiver/otelarrowreceiver/config.go Outdated Show resolved Hide resolved
receiver/otelarrowreceiver/internal/logs/otlp.go Outdated Show resolved Hide resolved
receiver/otelarrowreceiver/internal/metrics/otlp.go Outdated Show resolved Hide resolved
receiver/otelarrowreceiver/internal/trace/otlp.go Outdated Show resolved Hide resolved
receiver/otelarrowreceiver/factory.go Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Sep 17, 2024

I'm not sure what's going on with prometheus-compliance-tests / prometheus-compliance-tests but it's blocking these three otherwise ready-to-merge OTel-Arrow PRs:

#35021
#35225
#34742

@crobert-1
Copy link
Member

I'm not sure what's going on with prometheus-compliance-tests / prometheus-compliance-tests

It's frequency of #35119, unrelated to this change. 👍

@moh-osman3 moh-osman3 requested a review from a team as a code owner September 18, 2024 22:43
@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal/otelarrow ready to merge Code review completed; ready to merge by maintainers receiver/otelarrow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants