-
Notifications
You must be signed in to change notification settings - Fork 637
Allow tuning of logs batching behavior #4113
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
Conversation
| Processors: []ComponentID{ | ||
| "resource/pgbackrest", | ||
| "transform/pgbackrest_logs", | ||
| SubSecondBatchProcessor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to use this Processor? Seems like we could ditch it (I'm 10 files in)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still used in some metrics pipelines. 🤔 Worth a review later.
| var spec *v1beta1.InstrumentationLogsSpec | ||
| if inCluster != nil && inCluster.Spec.Instrumentation != nil { | ||
| spec = inCluster.Spec.Instrumentation.Logs | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use this var outside of the if feature.Enabled block, do we? And technically only use it once?
| MinRecords *int32 `json:"minRecords,omitempty"` | ||
| } | ||
|
|
||
| func (s *OpenTelemetryLogsBatchSpec) Default() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both kubebuilder defaults (when do those run, on admission?) and a default func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called by collector.NewConfig even when the section hasn't been filled out in the spec. IIUC, k8s doesn't fill out defaults inside missing optional fields.
Like,
---
# input from user:
instrumentation: {} # `batches` is missing and optional, no default; k8s does not add it
---
# input from user:
instrumentation:
batches: null # missing; stays null; nothing happens with defaults inside
---
# input from user:
instrumentation:
batches: {} # present; some fields inside have defaults
# k8s sets and stores defaults:
instrumentation:
batches:
maxDelay: 200ms
minRecords: 8192
---
# similarly, input:
instrumentation:
batches:
maxDelay: 1min
# k8s sets and stores defaults:
instrumentation:
batches:
maxDelay: 1m
minRecords: 8192📝 Default() used to satisfy some interface in controller-runtime for webhooks, but that style didn't work well for all webhooks. [Migrating]
benjaminjb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers (and happy to combine those pgadmin pipelines in my pgadmin PR)
Batches are an important component to shipping logs efficiently. Issue: PGO-2176
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
Batches of logs are exported every 200ms.
What is the new behavior (if this is a feature change)?
Users can configure the size and frequency of batches via the API. Documentation on each field explains what they do and why each is useful.
Other Information:
Issue: PGO-2176