Repository navigation
feat(search-events): correctly process attribute context in tests + deprecated attribute evals - #1440
nikkikapadia wants to merge 1 commit into
Conversation
…eprecated attribute evals
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
This generally looks good to me as a toolkit novice! I ran it through Claude and it produced a collection of reports that I narrowed down to a bunch of questions & nitpicks. So you shouldn't trust my review at all, this is just for me learning 😄
| 7. Use datasetAttributes substringMatch, query, and attributeTypes for targeted lookup when broad field discovery is truncated | ||
| 8. For non-replay datasets, call validateSearch after constructing the candidate request. If invalid, fix and validate again in this same pass | ||
| 9. NEVER replace a structured field:value filter with message/log.body/full-text matching. If an explicit field is unavailable on the dataset, keep it and let validation fail instead of inventing a weaker query | ||
| 10. If datasetAttributes lists a field under Deprecated Fields, use its replacement instead, even when the deprecated name appears in the guidance or examples in this prompt |
There was a problem hiding this comment.
[Question] Nothing in production passes context: true to fetchCustomAttributes. Is that something we handle separately?
| * truncated off the end of the listing, and the swapped-out fields are | ||
| * returned separately so the agent can be told not to use them. | ||
| */ | ||
| function preferReplacementFields(fields: Record<string, AttributeDescriptor>): { |
There was a problem hiding this comment.
[Question] I think this swap goes the opposite direction from Sentry's own handling? When a deprecated attribute and its replacement are both in the same RPC page, the attributes endpoint hides the replacement and keeps the source (_replacement_superseded_by_present_source in organization_trace_item_attributes.py). Is that something we need to worry about?
And/or, should this have direct unit tests?
| if (context?.isDeprecated) { | ||
| parts.push( | ||
| context.replacementAttribute | ||
| ? `DEPRECATED: use ${context.replacementAttribute} instead.` |
There was a problem hiding this comment.
[Bug?] This hint can steer the agent somewhere unhelpful when the replacement isn't in the list:
- 13 conventions are deprecated with no backfill/normalize status (e.g.
http.host→server.address,route→http.route), so the replacement may have no data db.params/db.sql.bindingshave the templated replacementdb.query.parameter.<key>, which can't be queried literally- On logs, numeric attributes come back as
tags[http.status_code,number]with replacementhttp.response.status_code. But the bare/directhttp.response.status_coderesolves as a string attribute on logs. Sohttp.response.status_code:503misses the numeric data. - On spans,
transactionwould read "DEPRECATED: use sentry.segment.name instead.", but the endpoint re-aliasessentry.segment.nametotransaction.environment/releaseget the same label even though they resolve to the same storage assentry.environment/sentry.release.
Should we maybe...
- ...give some kind of "if (thing) is possible" treatment? (or do agents just know to not fully trust these hints?)
- ...if this is legit, treat these mismatches as bugs... somewhere?
| ${replacedFields.length > 30 ? `\n... and ${replacedFields.length - 30} more deprecated fields` : ""}` | ||
| : "" | ||
| } | ||
| Recommended Fields for ${dataset}: |
There was a problem hiding this comment.
[Bug] Recommended Fields and the EXAMPLE QUERIES below aren't filtered against replacedKeys, so the same output can say "do NOT use http.method" and then show has:http.method as an example. Rule 10 covers this for the prompt but just for the Deprecated Fields section.
Filtering or rewriting those here might be more reliable. And either way, something to test maybe?
| type: attributeType, | ||
| attributeSource, | ||
| ...(secondaryAliases ? { secondaryAliases } : {}), | ||
| context, |
There was a problem hiding this comment.
[Style] Super nitty nit, feel free to ignore: but the line just above this only sets secondaryAliases if it's defined. Teeny little inconsistency.
Also, to nitpick everyone and everything, I personally like:
...(value && { value })...just for being a little more succinct. 😄
(absolutely not a blocker ofc)
| name: "datasetAttributes", | ||
| arguments: { | ||
| dataset: "spans", | ||
| substringMatch: "http.method", |
There was a problem hiding this comment.
[Testing] With params: "fuzzy", this requires a datasetAttributes call whose substringMatch contains "http.method", so a run that only calls { dataset: "spans" } and still produces the right query scores lower. The with-context case doesn't require it. Was that intentional?
| return [ | ||
| { | ||
| // EVENTUALLY Context marks http.method as deprecated in favor of http.request.method | ||
| // Context marks http.method as deprecated in favor of http.request.method |
There was a problem hiding this comment.
[Docs] Missing also updating the the suite-level comment above this?
This branch was successfully deployed
2 participants
The main purpose of this PR was to make sure that our evals with the attribute context take deprecated attributes into account and pick the replacement instead.
To do this, the attribute context we are providing in the mocks needed to be properly handled, parsed and passed to the agent in a way that it will understand. We've gone through and excluded attributes that have been deprecated from the list of attributes the agent sees and then also list out the deprecated fields and what its replacement attribute is.
I've adjusted some unit tests to reflect these changes as well.
Closes EXP-1288