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

ref(metrics): Remove performance.index-tag-values option flag #53870

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

john-z-yang
Copy link
Member

Overview

Removes the performance.index-tag-values option flag since performance metrics should always never index their tag values.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 31, 2023
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #53870 (5906d69) into master (263a737) will decrease coverage by 0.01%.
Report is 29 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 5906d69 differs from pull request most recent head 8fe89fb. Consider uploading reports for the commit 8fe89fb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #53870      +/-   ##
==========================================
- Coverage   79.53%   79.53%   -0.01%     
==========================================
  Files        4965     4965              
  Lines      209829   209827       -2     
  Branches    35721    35721              
==========================================
- Hits       166893   166889       -4     
- Misses      37812    37813       +1     
- Partials     5124     5125       +1     
Files Changed Coverage Δ
src/sentry/options/defaults.py 100.00% <ø> (ø)
src/sentry/sentry_metrics/configuration.py 100.00% <100.00%> (ø)
...try/sentry_metrics/consumers/indexer/processing.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@john-z-yang john-z-yang changed the title ref(metrics): Remove performance.index-tag-values option flag ref(metrics): Remove performance.index-tag-values option flag Jul 31, 2023
@john-z-yang john-z-yang marked this pull request as ready for review July 31, 2023 20:12
@john-z-yang john-z-yang requested a review from a team as a code owner July 31, 2023 20:12
@@ -42,7 +42,7 @@ class MetricsIngestConfiguration:
cardinality_limiter_cluster_options: Mapping[str, Any]
cardinality_limiter_namespace: str

index_tag_values_option_name: Optional[str] = None
should_index_tag_values: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

small q, should this be Optional[bool] = False?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it should, since the default is set to False and it should either be True or False and never None

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Though it does not change the behavior, it might be better to not have a default. Let each configuration explicitly set it following the general rule Explicit is better than implicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thank you!

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.

Left a minor comment. But approving since the changes look good.

@@ -42,7 +42,7 @@ class MetricsIngestConfiguration:
cardinality_limiter_cluster_options: Mapping[str, Any]
cardinality_limiter_namespace: str

index_tag_values_option_name: Optional[str] = None
should_index_tag_values: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Though it does not change the behavior, it might be better to not have a default. Let each configuration explicitly set it following the general rule Explicit is better than implicit.

@john-z-yang john-z-yang enabled auto-merge (squash) August 1, 2023 20:26
@john-z-yang john-z-yang merged commit 65dd0d8 into master Aug 1, 2023
@john-z-yang john-z-yang deleted the john/remove-itv branch August 1, 2023 20:41
jan-auer added a commit that referenced this pull request Aug 2, 2023
* master: (660 commits)
  fix(starfish): Improve time spent column formatting (#53989)
  fix(starfish): Improve span table columns (#53988)
  fix(starfish): Remove unnecessary wrapper around span description (#53986)
  ref(getting-started-docs): Make project deletion onBack enabled by default (#54020)
  ref(getting-started-docs): Make project deletion onBack enabled by default (#54019)
  chore(spans): Add feature flag for span extraction (#53925)
  ref(backpressure): Bump rabbit http request timeout (#54014)
  chore(test): Update tests/js/sentry-test/charts.tsx to TypeScript (#54010)
  ref(merged-pr-comments): Add a couple of metrics to track successes (#53981)
  fix(hc): Fix split DB test failures (#53907)
  migration(crons): Backfill next check-in latest (#53984)
  ref(crons): Match environment label color to icon color in timeline row (#53977)
  Stop publishing to PyPI (#53980)
  ref(alerts): Soft deprecate projectalertruledetails delete method (#53970)
  fix(hc): Stabilize OrganizationRepositoryDeleteTest (#53668)
  ref(replays): change widgets to be rage & dead clicks (#53982)
  fix(github-growth): catch and raise RepoExistsError (#53946)
  feat(root-cause-analysis): Add feature flag (#53955)
  feat(helpful-event): Add back trace.sampled to sort (#53936)
  ref(metrics): Remove `performance.index-tag-values` option flag (#53870)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants