-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: improved keys adjustment logic #9927
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors the field key adjustment logic in both logs and traces query statement builders by extracting the inline matching logic into a dedicated adjustKey method. The refactoring improves modularity and testability while maintaining the same functional behavior.
Key changes:
- Introduced new
adjustKeymethod in bothlogQueryStatementBuilderandtraceQueryStatementBuilderto handle key matching, context/type adjustment, and materialization logic in a modular way - Added comprehensive test coverage with
TestAdjustKeyandTestAdjustKeystests covering edge cases like intrinsic field collisions, mixed materialization states, and unknown fields - Expanded test data to include new scenarios such as
mixed.materialization.key,severity_textattribute collision, and various multi-context field cases
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/telemetrylogs/statement_builder.go | Extracts inline key adjustment logic into new adjustKey method for logs query builder |
| pkg/telemetrytraces/statement_builder.go | Extracts inline key adjustment logic into new adjustKey method for traces query builder; includes minor formatting fix |
| pkg/telemetrylogs/stmt_builder_test.go | Adds TestAdjustKey with 11 test cases covering various key adjustment scenarios |
| pkg/telemetrytraces/stmt_builder_test.go | Adds TestAdjustKey (15 cases) and TestAdjustKeys (4 cases), plus 3 new integration test cases |
| pkg/telemetrylogs/test_data.go | Adds test data for severity_text, mixed.materialization.key, multi.mat.key, mat.key, and materialized.key.name |
| pkg/telemetrytraces/test_data.go | Adds test data for duration_nano attribute and mixed.materialization.key scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
pkg/telemetrylogs/statement_builder.go:132
getKeySelectorsnow passesFieldDataType/FieldContextfrom the request into metadata key lookup. The metadata store uses these selector fields to filter which key tables are queried and which rows match; if a saved dashboard/query has an incorrect context/data type, the correct keys may not be returned and later key-adjustment logic can’t recover. Consider leaving selector context/data type unspecified for this prefetch (broad match), then resolving the exact key after adjustment (or refetching with narrowed selectors).
for idx := range query.GroupBy {
groupBy := query.GroupBy[idx]
keySelectors = append(keySelectors, &telemetrytypes.FieldKeySelector{
Name: groupBy.Name,
Signal: telemetrytypes.SignalLogs,
FieldContext: groupBy.FieldContext,
FieldDataType: groupBy.FieldDataType,
})
}
for idx := range query.SelectFields {
selectField := query.SelectFields[idx]
keySelectors = append(keySelectors, &telemetrytypes.FieldKeySelector{
Name: selectField.Name,
Signal: telemetrytypes.SignalLogs,
FieldContext: selectField.FieldContext,
FieldDataType: selectField.FieldDataType,
})
}
for idx := range query.Order {
keySelectors = append(keySelectors, &telemetrytypes.FieldKeySelector{
Name: query.Order[idx].Key.Name,
Signal: telemetrytypes.SignalLogs,
FieldContext: query.Order[idx].Key.FieldContext,
FieldDataType: query.Order[idx].Key.FieldDataType,
})
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
srikanthccv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
📄 Summary
This pull request introduces a new utility file,
collision.go, to thequerybuilderpackage, which provides robust logic for handling duplicate and ambiguous field keys in query builder queries. The new functions ensure that queries are more resilient to naming collisions and ambiguities, especially when dealing with JSON and promoted fields. Additionally, several test cases have been updated to reflect improved handling and warning messages for ambiguous keys.Key changes include:
New functionality for key handling:
collision.gotopkg/querybuilder, introducing functions likeAdjustDuplicateKeys,AdjustKey, andAdjustKeysForAliasExpressionsto deduplicate keys, resolve ambiguities, and adjust key representations in queries. These utilities ensure that each key inSelectFields,GroupBy, andOrderByis unique and as generic as possible, and help clarify ambiguous or aliased fields.Test updates for improved key handling:
json_stmt_builder_test.goto use unprefixed field names (e.g.,"user.age"instead of"body.user.age") in group by clauses, reflecting changes in how keys are handled and represented in queries. [1] [2] [3]jsondatatypeinformation, making ambiguity warnings more descriptive and aligned with the new key handling logic. [1] [2] [3]✅ Changes
🏷️ Required: Add Relevant Labels
ex:
frontendbackenddevopsbugenhancementuitest👥 Reviewers
🧪 How to Test
🔍 Related Issues
Closes #
https://github.com/SigNoz/engineering-pod/issues/3189
📸 Screenshots / Screen Recording (if applicable / mandatory for UI related changes)
📋 Checklist
👀 Notes for Reviewers
Note
Introduces generic key normalization/deduplication and smarter key resolution across queries.
pkg/querybuilder/collision.gowithAdjustDuplicateKeys,AdjustKey, and alias-awareAdjustKeysForAliasExpressionsto merge conflicting contexts/data types and reconcile intrinsic/calculated vs metadata keysorderBy; adjustsorderBykeys to alias form for non-raw requestsTelemetryFieldKey.String()and addsEqual, enabling clearer ambiguity warnings (now includejsondatatype,materialized, indexes)body.prefixes, add mixed materialization cases)Written by Cursor Bugbot for commit afea85a. This will update automatically on new commits. Configure here.