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

fix: update elastic.events.queued with the correct number of queued events #150

Merged
merged 6 commits into from
May 6, 2024

Conversation

kruskall
Copy link
Member

@kruskall kruskall commented Apr 11, 2024

Because of document retries the number of queued events after a flush request can be >0 if the returned 429.
To fix the issue we only remove (failed+indexed) from the queued event metric after a flush request.

Closes #150

…vents

Because of document retries the number of queued events after
a flush request can be >0 if the returned 429.
To fix the issue we only remove (failed+indexed) from the queued
event metric after a flush request.
@kruskall kruskall requested a review from a team as a code owner April 11, 2024 13:15
@elastic-apm-tech elastic-apm-tech added the safe-to-test Automated label for running bench-diff on forked PRs label Apr 11, 2024
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Can you please add a test to verify this works as expected?

@kruskall
Copy link
Member Author

Thanks for the fix. Can you please add a test to verify this works as expected?

I tried but it seems to require non-trivial work. I'd prefer to open a followup issue for that without bocking this PR.

@marclop
Copy link
Contributor

marclop commented Apr 16, 2024

I'm not convinced about pushing out tests for a bugfix. What's non-trivial about the test that needs to be added?

@simitt
Copy link

simitt commented May 1, 2024

@kruskall could you follow up on the last comment regarding the complexity of adding a test? I'm keen on getting this in, but right now the PR is stalling without additional tests.

@kruskall
Copy link
Member Author

kruskall commented May 2, 2024

I've added a test.

We have no test to validate metrics when document retries are enabled and can't really control the flush/retry behaviour so I had to "cheat":

  • only use 1 max request
  • use an explicit flush bytes to control the flush behaviour (this is fragile and can break really easily if we change something)

@kruskall
Copy link
Member Author

kruskall commented May 2, 2024

Before the fix the test fails and returns an Active and queued metric of -1

@kruskall kruskall requested a review from a team May 2, 2024 13:44
appender.go Outdated Show resolved Hide resolved
@kruskall kruskall requested a review from vikmik May 6, 2024 16:27
@kruskall kruskall merged commit 7d54164 into elastic:main May 6, 2024
5 checks passed
mx-psi referenced this pull request in open-telemetry/opentelemetry-collector-contrib May 14, 2024
….1 (#33026)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/elastic/go-docappender/v2](https://github.com/elastic/go-docappender)
| `v2.1.0` -> `v2.1.1` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2felastic%2fgo-docappender%2fv2/v2.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2felastic%2fgo-docappender%2fv2/v2.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2felastic%2fgo-docappender%2fv2/v2.1.0/v2.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2felastic%2fgo-docappender%2fv2/v2.1.0/v2.1.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>elastic/go-docappender
(github.com/elastic/go-docappender/v2)</summary>

###
[`v2.1.1`](https://github.com/elastic/go-docappender/releases/tag/v2.1.1)

[Compare
Source](https://github.com/elastic/go-docappender/compare/v2.1.0...v2.1.1)

##### What's Changed

- fix: update elastic.events.queued with the correct number of queued
events by [@&#8203;kruskall](https://github.com/kruskall) in
[https://github.com/elastic/go-docappender/pull/150](https://github.com/elastic/go-docappender/pull/150)

**Full Changelog**:
elastic/go-docappender@v2.1.0...v2.1.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNTEuMiIsInVwZGF0ZWRJblZlciI6IjM3LjM1MS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJyZW5vdmF0ZWJvdCJdfQ==-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
@kruskall kruskall deleted the fix/metric-queued-negative branch May 19, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe-to-test Automated label for running bench-diff on forked PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants