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

bulk_indexer: track all response status codes #177

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

marclop
Copy link
Contributor

@marclop marclop commented Jun 6, 2024

Updates the bulk_indexer to track all response status codes and report them appropriately as metrics. Also refactors tests to make them more succinct.

Closes #172

Updates the bulk_indexer to track all response status codes and report
them appropriately as metrics. Also refactors tests to make them more
succinct.

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
@marclop marclop requested a review from a team as a code owner June 6, 2024 10:31
@obltmachine obltmachine added the safe-to-test Automated label for running bench-diff on forked PRs label Jun 6, 2024
Comment on lines +542 to +547
type errorFlushFailed struct {
resp string
statusCode int
tooMany bool
clientError bool
serverError bool

Choose a reason for hiding this comment

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

nice 👍🏼

Choose a reason for hiding this comment

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

one other option would be to have a typed error for each case, and then wrapping them into an error returned on the specific branch, but we can always change that later

@inge4pres inge4pres self-requested a review June 11, 2024 17:17
inge4pres
inge4pres previously approved these changes Jun 11, 2024
Copy link

@inge4pres inge4pres left a comment

Choose a reason for hiding this comment

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

Thanks 👍🏼

Copy link
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

nit to test number of docs != requests to ensure that the right unit is used.

appender_test.go Outdated Show resolved Hide resolved
appender_test.go Outdated Show resolved Hide resolved
bulk_indexer.go Outdated
if res.StatusCode == http.StatusTooManyRequests {
return resp, errorTooManyRequests{res: res}
e := errorFlushFailed{resp: res.String(), statusCode: res.StatusCode}
if res.StatusCode >= 400 {
Copy link
Member

Choose a reason for hiding this comment

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

go-elasticsearch client treats StatusCode > 299 as Error here. I'm not sure how relevant handling of 3xx is, but right now any 3xx will be skipped in here

appender_test.go Outdated Show resolved Hide resolved
1pkg
1pkg previously approved these changes Jun 11, 2024
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
@marclop marclop dismissed stale reviews from 1pkg and inge4pres via 1442cc1 June 12, 2024 09:20
@marclop marclop requested a review from a team as a code owner June 12, 2024 09:20
bulk_indexer.go Outdated
if res.StatusCode == http.StatusTooManyRequests {
return resp, errorTooManyRequests{res: res}
e := errorFlushFailed{resp: res.String(), statusCode: res.StatusCode}
if res.StatusCode >= 400 {
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we have the switch case here as well? I find it more readable than nested if. Here we'll get the added benefit of fewer return statements.

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
@marclop marclop merged commit a1b25d4 into elastic:main Jun 12, 2024
5 checks passed
@marclop marclop deleted the b/track-all-metrics branch June 12, 2024 14:49
codeboten pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Jul 9, 2024
[![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.4` -> `v2.2.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2felastic%2fgo-docappender%2fv2/v2.2.0?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.2.0?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.4/v2.2.0?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.4/v2.2.0?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.2.0`](https://github.com/elastic/go-docappender/releases/tag/v2.2.0)

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

##### What's Changed

- fix: update CODEOWNERS by
[@&#8203;kruskall](https://github.com/kruskall) in
[elastic/go-docappender#181
- bulk_indexer: track all response status codes by
[@&#8203;marclop](https://github.com/marclop) in
[elastic/go-docappender#177
- build(deps): bump github.com/klauspost/compress from 1.17.8 to 1.17.9
by [@&#8203;dependabot](https://github.com/dependabot) in
[elastic/go-docappender#183
- add otel tracing support by
[@&#8203;endorama](https://github.com/endorama) in
[elastic/go-docappender#182

##### New Contributors

- [@&#8203;endorama](https://github.com/endorama) made their first
contribution in
[elastic/go-docappender#182

**Full Changelog**:
elastic/go-docappender@v2.1.4...v2.2.0

</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:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjUuMSIsInVwZGF0ZWRJblZlciI6IjM3LjQyNS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJyZW5vdmF0ZWJvdCJdfQ==-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
Co-authored-by: Yang Song <songy23@users.noreply.github.com>
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jul 11, 2024
…lemetry#33967)

[![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.4` -> `v2.2.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2felastic%2fgo-docappender%2fv2/v2.2.0?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.2.0?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.4/v2.2.0?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.4/v2.2.0?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.2.0`](https://github.com/elastic/go-docappender/releases/tag/v2.2.0)

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

##### What's Changed

- fix: update CODEOWNERS by
[@&open-telemetry#8203;kruskall](https://github.com/kruskall) in
[elastic/go-docappender#181
- bulk_indexer: track all response status codes by
[@&open-telemetry#8203;marclop](https://github.com/marclop) in
[elastic/go-docappender#177
- build(deps): bump github.com/klauspost/compress from 1.17.8 to 1.17.9
by [@&open-telemetry#8203;dependabot](https://github.com/dependabot) in
[elastic/go-docappender#183
- add otel tracing support by
[@&open-telemetry#8203;endorama](https://github.com/endorama) in
[elastic/go-docappender#182

##### New Contributors

- [@&open-telemetry#8203;endorama](https://github.com/endorama) made their first
contribution in
[elastic/go-docappender#182

**Full Changelog**:
elastic/go-docappender@v2.1.4...v2.2.0

</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:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjUuMSIsInVwZGF0ZWRJblZlciI6IjM3LjQyNS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJyZW5vdmF0ZWJvdCJdfQ==-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
Co-authored-by: Yang Song <songy23@users.noreply.github.com>
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.

Missing bulk request metrics on error
5 participants