feat(discover): allow querying for cache.item_size and cache.hit

[DominikB2014]

This PR complements the relay PR getsentry/relay#3371

It allows us to query for cache.item_size metric and cache.hit field via discover api through spansMetrics dataset.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.72%. Comparing base (b50875c) to head (df88618).

❗ Current head df88618 differs from pull request most recent head fab1bc9. Consider uploading reports for the commit fab1bc9 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #68788       +/-   ##
===========================================
+ Coverage   42.52%   79.72%   +37.20%     
===========================================
  Files        5435     6419      +984     
  Lines      228486   284522    +56036     
  Branches    39297    49001     +9704     
===========================================
+ Hits        97158   226843   +129685     
+ Misses     130430    57312    -73118     
+ Partials      898      367      -531     

Files	Coverage Δ
src/sentry/search/events/constants.py	100.00% <ø> (ø)
src/sentry/sentry_metrics/indexer/strings.py	100.00% <ø> (+59.15%)	⬆️
src/sentry/snuba/metrics/naming_layer/mri.py	95.38% <100.00%> (+23.21%)	⬆️
src/sentry/snuba/metrics/naming_layer/public.py	100.00% <100.00%> (ø)
src/sentry/testutils/cases.py	89.11% <ø> (+57.11%)	⬆️

... and 3771 files with indirect coverage changes

[DominikB2014]

It seems like this is failing because we can't modify strings.py https://github.com/getsentry/sentry/actions/runs/8711767062/job/23896615105#step:4:1542

Without the strings.py entry here
https://github.com/getsentry/sentry/pull/68788/files#diff-5ef5f3794ce0417331dd489ccb931ca76a275f37a470840ade5fc482999a57e7R314

We get the following error when trying query for cache.hit in discover
{"detail":"Column cache.hit was not found in metrics indexer"}
@wmak @phacops any ideas if there's some new procedure to get to this to work?

[wedamija]

I'm not too up on the latest with metrics but won't the item size be much too high cardinality?

[DominikB2014]

I'm not too up on the latest with metrics but won't the item size be much too high cardinality?

@wedamija item_size is just a metric, and we don't tag on item_size so it should be okay for cardinality. (Kinda like we have a span.duration metrics, but we don't tag metrics with span.duration)

We do tag metrics with cache.hit, but this only has 3 possible values (true, false, "")

[evanh]

we get the following error when trying query for cache.hit in discover

This is confusing to me, I didn't realize you could query metrics in discover, since discover is an event store.

That being said, there shouldn't be any need to add anything to strings.py. That error means that the metric/tag key has never been seen before, which shouldn't be happening. I would add the cache.hit tag to your test and see if that works.

[DominikB2014]

@evanh

This is confusing to me, I didn't realize you could query metrics in discover, since discover is an event store.

Yeah, we pretty much use the discover api (which I believe the api has some translation layer to get the right metrics if they aren't in the discover dataset). We use the api exclusively to query for metrics in all the performance UIs

That being said, there shouldn't be any need to add anything to strings.py. That error means that the metric/tag key has never been seen before, which shouldn't be happening. I would add the cache.hit tag to your test and see if that works.

Unfortunately the tests still fail :(, seems like something under the hood in discover that requires the strings.py entries

[phacops]

This suggest we're querying the indexer first to know if it's a registered tag before we consider querying for it. Since we removed the need to register a tag, this check needs to go away.

The consequence of this is we could potentially query a tag that has no data associated with it. I think I'm fine with this.

[evanh]

something under the hood in discover that requires the strings.py entries

There is a bit of a misunderstanding here I think. Something in discover requires that the string cache.hit be indexed. Using the strings.py file is one way to index a string, but the other way is to add the string to the indexer DB. You can use a function like https://github.com/getsentry/sentry/blob/master/src/sentry/sentry_metrics/utils.py#L141 to do that.

The store_metric function in the tests should be doing that already, so if that isn't working it sounds like a test failure instead of an indexer failure. Either way, modifying strings.py shouldn't be a requirement to get this working.

[DominikB2014]

@evanh turns out this was the problem, we were setting the wrong useCaseId, and the metric/tag index would fail to resolve when we queried for them, tests pass just fine now!