Skip to content

Send new label 'status' for ingester failures#4442

Merged
alvinlin123 merged 4 commits into
cortexproject:masterfrom
danielblando:ingester-error-status-family
Sep 15, 2021
Merged

Send new label 'status' for ingester failures#4442
alvinlin123 merged 4 commits into
cortexproject:masterfrom
danielblando:ingester-error-status-family

Conversation

@danielblando
Copy link
Copy Markdown
Contributor

@danielblando danielblando commented Aug 24, 2021 •
edited
Loading

What this PR does:
Add new label status to metric cortex_distributor_ingester_append_failures_total
This is intended to differentiate errors sent from ingester

Which issue(s) this PR fixes:
Fixes #4441

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@danielblando danielblando force-pushed the ingester-error-status-family branch from 4aeb956 to cb69329 Compare August 24, 2021 02:03
alvinlin123
alvinlin123 reviewed Aug 24, 2021
View reviewed changes
Comment thread CHANGELOG.md Outdated
@danielblando danielblando force-pushed the ingester-error-status-family branch from b06e8c2 to 3510532 Compare August 25, 2021 04:48
pracucci
pracucci reviewed Aug 26, 2021
View reviewed changes
Comment thread pkg/distributor/distributor.go Outdated
Comment thread pkg/distributor/distributor.go Outdated
alvinlin123
alvinlin123 approved these changes Aug 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@alvinlin123 alvinlin123 left a comment

Choose a reason for hiding this comment

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

LGTM, giving you approval to kick off workflow :)

@danielblando danielblando force-pushed the ingester-error-status-family branch from 848022a to 3be2730 Compare August 26, 2021 20:30
@alvinlin123
Copy link
Copy Markdown
Contributor

alvinlin123 commented Aug 26, 2021

@pracucci, I am wondering how come I don't see a "Approve and run" button in the workflow panel like https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks suggests?

@danielblando danielblando force-pushed the ingester-error-status-family branch from 3be2730 to e829b47 Compare August 27, 2021 00:49
pracucci
pracucci approved these changes Aug 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM (modulo a nit)

Comment thread CHANGELOG.md Outdated
@danielblando danielblando force-pushed the ingester-error-status-family branch from e829b47 to 70a966e Compare August 27, 2021 15:40
@danielblando
Copy link
Copy Markdown
Contributor Author

danielblando commented Aug 30, 2021

@pracucci do you mind approving again? That would make the workflow to run

@danielblando
Copy link
Copy Markdown
Contributor Author

danielblando commented Aug 30, 2021

Hmm not sure actually what happened. I think the PR just need an update to fire the workflow. It started after my previous comment.

@pstibrany
Copy link
Copy Markdown
Contributor

pstibrany commented Aug 30, 2021

Hmm not sure actually what happened. I think the PR just need an update to fire the workflow. It started after my previous comment.

Sorry for the mystery here… I started the check manually after seeing your previous comment.

@danielblando
Copy link
Copy Markdown
Contributor Author

danielblando commented Aug 30, 2021

Cool @pstibrany. Thanks

@bboreham bboreham changed the title Send new label statusFamily for ingestor failures Send new label 'status' for ingester failures Sep 1, 2021
bboreham
bboreham approved these changes Sep 1, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

bboreham
bboreham reviewed Sep 2, 2021
View reviewed changes
Comment thread pkg/distributor/distributor_test.go Outdated
Copy link
Copy Markdown
Contributor

@bboreham bboreham Sep 2, 2021

Choose a reason for hiding this comment

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

Sorry, now I come to read this again I can't see why this is called errFail.
It looks like this is the error that will be returned if not happy - is that correct?
Logically a 4xx error ought to be associated with the request not the ingester.

Copy link
Copy Markdown
Contributor Author

@danielblando danielblando Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@bboreham Yes, you are right. I was trying to use the same pattern we already have for the test globally, but as this can be a 4xx or a 5xx it does not necessarily mean an error. I changed to 'failResp'. Let me know if you have another suggestion.

@danielblando danielblando force-pushed the ingester-error-status-family branch 2 times, most recently from 6ecc089 to 3ed8e0f Compare September 3, 2021 06:54
@danielblando
Send new label statusFamily for ingestor failures
86e92d5 
Signed-off-by: Daniel Blando <ddeluigg@amazon.com>
@danielblando
update changelog
58f08a5 
Signed-off-by: Daniel Blando <ddeluigg@amazon.com>
@danielblando
Change changelog
5f788a1 
Signed-off-by: Daniel Blando <ddeluigg@amazon.com>
@danielblando
Update label name
3efe528 
Signed-off-by: Daniel Blando <ddeluigg@amazon.com>
@danielblando danielblando force-pushed the ingester-error-status-family branch from 3ed8e0f to 3efe528 Compare September 3, 2021 07:02
@alvinlin123 alvinlin123 merged commit 76d5b3c into cortexproject:master Sep 15, 2021
@danielblando danielblando deleted the ingester-error-status-family branch September 15, 2021 17:48
srijan55 pushed a commit to srijan55/cortex that referenced this pull request Nov 26, 2021
@danielblando @srijan55
Send new label 'status' for ingester failures (cortexproject#4442)
9f45d35 
* Send new label statusFamily for ingestor failures

Signed-off-by: Daniel Blando <ddeluigg@amazon.com>

* update changelog

Signed-off-by: Daniel Blando <ddeluigg@amazon.com>

* Change changelog

Signed-off-by: Daniel Blando <ddeluigg@amazon.com>

* Update label name

Signed-off-by: Daniel Blando <ddeluigg@amazon.com>
Signed-off-by: Manish Kumar Gupta <manishkg@microsoft.com>
alvinlin123 pushed a commit to ac1214/cortex that referenced this pull request Jan 14, 2022
@danielblando @alvinlin123
Send new label 'status' for ingester failures (cortexproject#4442)
d3c33f2 
* Send new label statusFamily for ingestor failures

Signed-off-by: Daniel Blando <ddeluigg@amazon.com>

* update changelog

Signed-off-by: Daniel Blando <ddeluigg@amazon.com>

* Change changelog

Signed-off-by: Daniel Blando <ddeluigg@amazon.com>

* Update label name

Signed-off-by: Daniel Blando <ddeluigg@amazon.com>
Signed-off-by: Alvin Lin <alvinlin@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@bboreham bboreham bboreham approved these changes

@alvinlin123 alvinlin123 alvinlin123 approved these changes

@pracucci pracucci pracucci approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Metrics by error type when distributer fail to call ingester

5 participants

@danielblando @alvinlin123 @pstibrany @pracucci @bboreham