fix(otelconsumer): do not retry 401 errors from Elasticsearch#50261
Conversation
I'm not sure if it makes things better (less coupling) or worse (less explicit).
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
|
This pull request doesn't have a |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change updates otelconsumer.logsPublish to recognize errors exposing 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
TL;DRThe failures are not caused by the Remediation
Investigation detailsRoot Cause
Evidence
Verification
Follow-up
Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | noneWhat is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
Force merging, test failures are unrelated. |
|
@Mergifyio backport 8.19 9.3 9.4 |
✅ Backports have been createdDetails
Cherry-pick of fdea8cd has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of fdea8cd has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of fdea8cd has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
…m Elasticsearch (#50273) * fix(otelconsumer): do not retry 401 errors from Elasticsearch (#50261) * fix(otelconsumer): treat 401 errors from Elasticsearch as permanent * refactor: prevent tight coupling with docappender library I'm not sure if it makes things better (less coupling) or worse (less explicit). * libbeat: mage check --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co> (cherry picked from commit fdea8cd) # Conflicts: # libbeat/otel/otelconsumer/otelconsumer.go * Fix conflict --------- Co-authored-by: Andrzej Stencel <andrzej.stencel@elastic.co> Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…c#50261) * fix(otelconsumer): treat 401 errors from Elasticsearch as permanent * refactor: prevent tight coupling with docappender library I'm not sure if it makes things better (less coupling) or worse (less explicit). * libbeat: mage check --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
|
Hey @cmacknz @andrzej-stencel 👋 We've been investigating e2e test failures in ECK (cloud-on-k8s) that started occurring after this PR was backported to 9.4. I wanted to share some context about a scenario where treating 401 as not retryable from the start causes data loss. Use-Case: Elasticsearch File-based Realm in KubernetesIn Kubernetes deployments managed by ECK, Elasticsearch uses a file-based realm for authentication. When ECK creates credentials for an Elastic Agent, at a high level:
There's an inherent delay between when the credentials are created and when Elasticsearch actually accepts them. During this window, any authentication attempt returns 401 - but this is a transient condition, not a permanent authentication failure. Why This MattersWith this change, when Agent components start producing data before ES has reloaded the file realm:
This is particularly problematic because some components (like filebeat monitoring its own logs) only generate certain events at startup. Once those events are dropped, they're never regenerated. IMO Expected BehaviorIn general, workloads should be resilient to transient errors during startup. A 401 while credentials are still propagating through the system is fundamentally different from a 401 caused by genuinely invalid credentials. ProposalI understand the original issue (beats#50217) was about tight-loop retries with zero backoff causing CPU saturation. That's a real problem that needed fixing. However, completely removing retry capability for 401 swings too far in the other direction. Is this commit part of the 9.4.0 upcoming release? More importantly is there any chance we can revert this PR in time? This change will cause a real issue for ECK users. |
It is part of 9.4.0 and there is still time to revert it. This breaking something that used to work suggest this is an accidental breaking change. Looking at the beat ES output logic here, it looks like 401s would not get retried at the exporter either. The collector in general wants 401s to be treated as permanent errors (though not yet the case for the ES exporter open-telemetry/opentelemetry-collector-contrib#47870). beats/libbeat/outputs/elasticsearch/client.go Lines 501 to 553 in bd70fc6 I wonder if the Filebeat guaranteed send feature for logs is doing the retry here, and this is bypassing that? #47892. This would not apply to metrics though. I would be in favour of reverting this to be safe while we work out how to handle this in a nicer way. We do not need to drop to avoid retrying rapidly, those options are seperate. |
|
Thanks for the prompt reply @cmacknz! I totally get the logic behind marking a 401 as a permanent error - it just feels like a more subtle approach to treat it as permanent only after some retry period with backoff, rather than immediately on the first occurrence. |
|
Revert PR #50341 |
…rors from Elasticsearch" (#50353) * Revert "fix(otelconsumer): do not retry 401 errors from Elasticsearch (#50261)" (#50341) This reverts commit fdea8cd. (cherry picked from commit 4ed1c3a) # Conflicts: # libbeat/otel/otelconsumer/otelconsumer.go * Fix conflict --------- Co-authored-by: Andrzej Stencel <andrzej.stencel@elastic.co> Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…c#50261) * fix(otelconsumer): treat 401 errors from Elasticsearch as permanent * refactor: prevent tight coupling with docappender library I'm not sure if it makes things better (less coupling) or worse (less explicit). * libbeat: mage check --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…elastic#50261)" (elastic#50341) This reverts commit fdea8cd.
…c#50261) * fix(otelconsumer): treat 401 errors from Elasticsearch as permanent * refactor: prevent tight coupling with docappender library I'm not sure if it makes things better (less coupling) or worse (less explicit). * libbeat: mage check --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…elastic#50261)" (elastic#50341) This reverts commit fdea8cd.
…c#50261) * fix(otelconsumer): treat 401 errors from Elasticsearch as permanent * refactor: prevent tight coupling with docappender library I'm not sure if it makes things better (less coupling) or worse (less explicit). * libbeat: mage check --------- Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
…elastic#50261)" (elastic#50341) This reverts commit fdea8cd.
Proposed commit message
Prevents retrying of
401 Unauthorizederrors from Elasticsearch in otelconsumer.This is a (hopefully temporary) workaround for the fact that the Elasticsearch exporter does not mark 401 errors from Elasticsearch as permanent, which it probably should.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration filesstresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.Disruptive User Impact
This change the behavior of Beat receivers to not retry 401 errors from Elasticsearch. I cannot imagine a scenario where someone would depend on this behavior, so this change should be non-consequential.
How to test this PR locally
Related issues