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

Unify blob size limit breach logging and metering #6250

Merged

Conversation

3vilhamster
Copy link
Member

@3vilhamster 3vilhamster commented Aug 23, 2024

What changed?
I introduced a new log and metric for the blob size limit breach.

Why?
We want to track blob size violations for each domain and be able to identify all failed workflows.

How did you test it?
Unit tests.

Potential risks
For misconfigured domains we can introduce a lot of logs, but that is not expected state.

Release notes
Introduced new metric for blob size violations:
blob_size_exceed_limit that can point to issues for a specific domain and operation.

Documentation Changes

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.98%. Comparing base (c0cd4c5) to head (0d86ea5).
Report is 4 commits behind head on master.

Additional details and impacted files
Files Coverage Δ
common/util.go 81.41% <100.00%> (+2.56%) ⬆️
service/frontend/api/handler.go 65.57% <100.00%> (+0.19%) ⬆️
service/history/decision/checker.go 79.49% <100.00%> (+0.08%) ⬆️
service/history/decision/handler.go 94.95% <100.00%> (+0.01%) ⬆️

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0cd4c5...0d86ea5. Read the comment docs.

common/util.go Outdated Show resolved Hide resolved
Comment on lines 1180 to +1181
taskToken.DomainID,
domainName,
Copy link
Member

@Groxx Groxx Aug 23, 2024

Choose a reason for hiding this comment

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

really makes me wish we had a common name-and-id pair :| we have access to both almost everywhere...

that'd be a bigger change tho, so 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. We do have a domain cache, but it is not propagated everywhere. I f we used DI, we could separate this component and make sure that domain cache is provided, but we don't for now.

common/util_test.go Outdated Show resolved Hide resolved
common/util_test.go Outdated Show resolved Hide resolved
common/util_test.go Outdated Show resolved Hide resolved
Copy link
Member

@Groxx Groxx left a comment

Choose a reason for hiding this comment

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

Some small Qs and nits but overall LGTM. Seems fine to merge as-is, everything's optional and only if you agree :)

@3vilhamster 3vilhamster merged commit 630013c into cadence-workflow:master Aug 26, 2024
20 checks passed
@3vilhamster 3vilhamster deleted the unified_blob_size_failure branch August 26, 2024 09:34
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.

2 participants