Skip to content

Conversation

@cbandy
Copy link
Member

@cbandy cbandy commented Mar 1, 2025

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

Type of Changes:

  • New feature

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

Processors: []ComponentID{
"resource/pgbackrest",
"transform/pgbackrest_logs",
SubSecondBatchProcessor,
Copy link
Contributor

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)

Copy link
Member Author

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.

Comment on lines +20 to +24
var spec *v1beta1.InstrumentationLogsSpec
if inCluster != nil && inCluster.Spec.Instrumentation != nil {
spec = inCluster.Spec.Instrumentation.Logs
}

Copy link
Contributor

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() {
Copy link
Contributor

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?

Copy link
Member Author

@cbandy cbandy Mar 3, 2025

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]

Copy link
Contributor

@benjaminjb benjaminjb left a 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
@cbandy cbandy enabled auto-merge (rebase) March 4, 2025 16:57
@cbandy cbandy merged commit 9ff9615 into CrunchyData:main Mar 4, 2025
18 checks passed
@cbandy cbandy deleted the batches branch March 4, 2025 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants