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

Get rid of noisy task adding failure log in matching service #5445

Conversation

taylanisikdemir
Copy link
Member

@taylanisikdemir taylanisikdemir commented Nov 9, 2023

What changed?
Failed to add task error log for remote sync match failed scenario seems to be too noisy (96% of matching logs). We log it for multiple cases such as

  • task forwarded from child partition but domain is not active in current cluster (ref)
  • task forwarded from child partition but couldn't be sync matched (ref)

There's already a metric cadence_errors_remote_syncmatch_failed_per_tl counting this specific case and it has the same breakdown as the log does

  • domain
  • tasklist name
  • operation (adddecision vs addactivity task)

So the change is to remove this log line. Other sub categories of Failed to add task error log will still be logged by the matching context's handleErr as an Uncategorized error. This seems fine to me but open for feedback here. We can keep those other sub types around and just get rid of remote sync match failed because it's a known type w/ its own counter.

Misc Change
Only signal taskReader if there's no error and no sync match found. Previously it was signaling even for sync matches but that looked unnecessary as nothing is written to persistence for sync match AFAICT. This can help reduce the unnecessary DB calls during sync matches.

Why?
Since this log entry adds no extra visibility and floods the logs, there's no point of keeping it.

How did you test it?
make test

Potential risks
Not signaling task reader for sync matches can have side effects I am not able to see. Looking forward to review feedback on this.

@CLAassistant
Copy link

CLAassistant commented Nov 9, 2023

CLA assistant check
All committers have signed the CLA.

@taylanisikdemir taylanisikdemir removed the request for review from davidporter-id-au November 13, 2023 17:44
@taylanisikdemir taylanisikdemir force-pushed the taylan/matching_add_task_noisy_errors branch from beed564 to 44e1210 Compare November 13, 2023 17:46
Copy link
Member

@davidporter-id-au davidporter-id-au left a comment

Choose a reason for hiding this comment

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

For higher-risk changes I'd suggest doing some integration-deployments, will discuss offline. otherwise LGTM

service/matching/taskListManager.go Show resolved Hide resolved
@taylanisikdemir taylanisikdemir force-pushed the taylan/matching_add_task_noisy_errors branch 2 times, most recently from 378a338 to 62eec96 Compare November 14, 2023 01:29
@taylanisikdemir taylanisikdemir force-pushed the taylan/matching_add_task_noisy_errors branch from 62eec96 to 1d329ca Compare November 14, 2023 03:19
@taylanisikdemir taylanisikdemir merged commit 10ae6c4 into cadence-workflow:master Nov 14, 2023
16 checks passed
@taylanisikdemir taylanisikdemir deleted the taylan/matching_add_task_noisy_errors branch November 14, 2023 16:47
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.

4 participants