feat: improved keys adjustment logic #9927
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 3
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
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.
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.
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.
| } else { | ||
| // Here we have a key which is an intrinsic field but also exists in the metadata with the same name | ||
| // cannot do anything, so just return | ||
| return |
There was a problem hiding this comment.
This situation should not be happening, if it does, it's likely an issue worth letting user know about it? And where do we handle it?
Let's take body, some users also send it as attribute, they almost always want the log body than attribute body. What do you think?
There was a problem hiding this comment.
| } | ||
|
|
||
| if len(matchingKeys) == 0 { | ||
| // we do not have any matching keys, most likely user made a mistake, let QB handle it |
There was a problem hiding this comment.
let QB handle it
What happens to invalid keys in selected fields, group by, order by? Will it create a valid query and predictable results?
|
discussed all the failure cases (that arise from the bad quality of data) here https://github.com/SigNoz/engineering-pod/issues/3193#issuecomment-3715954248. @tushar-signoz will write up a doc with details (and solutions to problems including the UX) and implement it. |
2 participants
📄 Summary
This pull request refactors and unifies the logic for adjusting field keys in telemetry logs and traces query builders. It introduces a new method to handle key adjustment in a more modular and testable way, adds comprehensive test coverage for this logic, and expands test data to cover more edge cases and scenarios.
Key changes include:
Refactoring and Logic Unification
adjustKeymethod in bothlogQueryStatementBuilderandtraceQueryStatementBuilder, replacing the previous inline logic for matching and adjusting field keys. This method now handles intrinsic, calculated, and metadata fields in a consistent and modular way. [1] [2] [3]Test Coverage Improvements
TestAdjustKeyunit test to thoroughly validate the newadjustKeylogic against a variety of scenarios, including intrinsic fields, metadata matches, materialization states, and unknown fields.Test Data Enhancements
buildCompleteFieldKeyMapandbuildCompleteFieldKeyMapCollisionto include additional cases such as mixed materialization, multiple matching keys, and new field names, ensuring the new logic is well-tested. [1] [2] [3] [4] [5]✅ 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
Unifies and hardens field key adjustment across logs and traces query builders.
adjustKeyand applies it inadjustKeysforSelectFields,GroupBy, andOrderto reconcile context/data type/materialization using intrinsic/calculated metadataTestAdjustKey(logs, traces) andTestAdjustKeys(traces) covering intrinsic vs metadata, mixed/multiple matches, unknown fields, and materializationmixed.materialization.key,multi.mat.key,mat.key,materialized.key.name, and additional cases (e.g.,severity_number, attributeduration_nano) to validate edge conditionsWritten by Cursor Bugbot for commit ec32181. This will update automatically on new commits. Configure here.