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

feat(meta): Add record_meta field to counters consumer #5680

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

evanh
Copy link
Member

@evanh evanh commented Mar 22, 2024

This field will be used by the materialized view to determine whether the row
should be processed into the meta table.

Depends on #5681

This field will be used by the materialized view to determine whether the row
should be processed into the meta table.
@evanh evanh requested a review from a team as a code owner March 22, 2024 14:30
@evanh evanh requested a review from a team March 22, 2024 14:30
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.93%. Comparing base (f5f9208) to head (9a6b53d).

✅ All tests successful. No failed tests found ☺️

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5680   +/-   ##
=======================================
  Coverage   89.92%   89.93%           
=======================================
  Files         898      898           
  Lines       43453    43453           
  Branches      299      299           
=======================================
+ Hits        39077    39078    +1     
+ Misses       4334     4333    -1     
  Partials       42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@nikhars nikhars left a comment

Choose a reason for hiding this comment

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

Since this is an optimization, would it be better to do it via an allowlist rather than a blacklist? That way we avoid new use cases being recorded in the meta table by default. Something like escalating issues causing unnecessary overhead on the meta table.

rust_snuba/src/processors/generic_metrics.rs Outdated Show resolved Hide resolved
@evanh
Copy link
Member Author

evanh commented Mar 22, 2024

Since this is an optimization, would it be better to do it via an allowlist rather than a blacklist?

I did it this way so that we can avoid a potential step in someone creating a new use case. This way, if a product person creates a new use case, they don't then have to take another step to get auto-complete, it just comes for free.

@evanh evanh merged commit 050c343 into master Mar 25, 2024
32 checks passed
@evanh evanh deleted the evanh/feat/add-record-meta-to-counters branch March 25, 2024 16:56
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.

3 participants