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

Merged
merged 28 commits into from
Sep 26, 2024

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/[email protected]
    Fixed in: encoding/[email protected]
    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
Copy link

@lquerel lquerel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor comments otherwise LGTM.

receiver/otelarrowreceiver/README.md Outdated Show resolved Hide resolved
receiver/otelarrowreceiver/config.go Show resolved Hide resolved
@djaglowski djaglowski merged commit c5aa92a into open-telemetry:main Sep 26, 2024
156 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 26, 2024
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this pull request Oct 4, 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.

6 participants