feat: improve collision handling#9895
Conversation
tushar-signoz
requested review from
srikanthccv and
therealpandey
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request enhances field mapping and column expression logic in the telemetry system to better handle field resolution when context or data type information is missing or ambiguous (collision scenarios). It introduces helper methods, improves error handling with suggestions for typos, and expands test coverage.
- Adds
populateMissingFieldContextAndDataTypefunction to intelligently infer missing field context and data type from available keys - Enhances
ColumnExpressionForto buildmultiIfSQL expressions when fields could exist in multiple contexts or data types - Includes
SelectFieldsin query key extraction to ensure all relevant fields are processed
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/types/telemetrytypes/field.go | Adds GetKeyTextFromFieldKey utility function to convert field keys back to text format |
| pkg/telemetrytraces/stmt_builder_test.go | Minor formatting fix (spacing adjustment) |
| pkg/telemetrytraces/field_mapper_test.go | Adds test coverage for ColumnExpressionFor with various scenarios including missing context and materialized columns |
| pkg/telemetrymetrics/field_mapper_test.go | Adds test coverage for ColumnExpressionFor including duplicate contexts and missing context fallback |
| pkg/telemetrymetrics/field_mapper.go | Renames unused keys parameter to _ for clarity |
| pkg/telemetrylogs/statement_builder.go | Updates getKeySelectors to include SelectFields and improves documentation |
| pkg/telemetrylogs/field_mapper_test.go | Renames test functions for clarity and adds comprehensive tests for field resolution scenarios |
| pkg/telemetrylogs/field_mapper.go | Implements core collision handling logic with populateMissingFieldContextAndDataType and enhanced ColumnExpressionFor |
| pkg/telemetrylogs/const.go | Adds LogsV2ResourceColumn constant for resource-level field resolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
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.
tushar-signoz
force-pushed
the
tvats-improve-collision-handling
branch
from
f6bb760 to
4092913
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 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>
| } else if len(keysForField) == 1 { | ||
| // we have a single key for the field, use it | ||
| field.FieldContext = keysForField[0].FieldContext | ||
| field.FieldDataType = keysForField[0].FieldDataType |
There was a problem hiding this comment.
User-specified field data type gets overwritten by key value
The populateMissingFieldContextAndDataType function's purpose (per its name and documentation) is to populate MISSING FieldContext and FieldDataType values. However, when there's exactly one key for a field, lines 325-326 unconditionally overwrite both values from the key, even if the user already specified one of them. For example, if a user explicitly specifies FieldDataType: String but the only key in the map has FieldDataType: Number, the user's explicit specification gets silently overwritten. This could cause queries to use incorrect data types when the user explicitly requests a specific type that differs from what exists in the keys map.
| } | ||
| } | ||
| if len(args) == 0 { | ||
| return "", errors.Wrapf(err, errors.TypeInvalidInput, errors.CodeInvalidInput, "field `%s` not found", field.Name) |
There was a problem hiding this comment.
Non-ErrColumnNotFound errors silently ignored producing invalid SQL
The ColumnExpressionFor function only checks for ErrColumnNotFound errors from FieldFor, but FieldFor can return several other error types (e.g., "only resource context fields are supported for json columns" at line 118). When such errors occur, colName is an empty string, the error check at line 191 evaluates to false, and the function silently returns invalid SQL like ASfieldname`` with an empty column expression. This can be triggered when populateMissingFieldContextAndDataType sets `FieldContext` to `Log` for the "resource" field (a JSON column), which then fails in `FieldFor` because JSON columns only support `Resource` context.
| // 3. if data type matches, consider this key | ||
| if (field.FieldContext == telemetrytypes.FieldContextUnspecified && field.FieldDataType == telemetrytypes.FieldDataTypeUnspecified) || | ||
| key.FieldContext == field.FieldContext || | ||
| key.FieldDataType == field.FieldDataType { |
There was a problem hiding this comment.
Filtering logic uses OR instead of AND, matching wrong keys
The filtering condition in populateMissingFieldContextAndDataType uses OR logic instead of AND. When a user specifies only FieldContext (e.g., Attribute) but leaves FieldDataType as Unspecified, the condition key.FieldDataType == field.FieldDataType incorrectly matches any key that also has Unspecified data type, regardless of whether its context matches. If a key with wrong context but matching Unspecified data type is the only one that passes filtering, it becomes the "single match" at line 343 and overwrites the user's explicit context specification. The condition uses || but the intended behavior requires && logic to ensure keys match ALL specified criteria rather than just one.
|
Superseded by #9927 |
📄 Summary
This pull request introduces significant improvements to the field mapping and column expression logic in the telemetry logs package, making field resolution more robust and flexible. It adds new helper methods, enhances error handling, and expands test coverage to ensure correctness in various field resolution scenarios, especially when context or data type is missing or ambiguous.
Key changes include:
Field resolution and mapping enhancements
FieldFor,ColumnFor, andColumnExpressionFormethods to thefieldMapperfor more granular and user-friendly field-to-column resolution, including improved error messages and support for aliasing in SQL expressions. [1] [2]populateMissingFieldContextAndDataTypeto infer missing field context and data type from available keys, improving the handling of ambiguous or incomplete field specifications.ColumnExpressionForto constructmultiIfexpressions for fields with multiple possible contexts or data types, and to suggest corrections for unrecognized field names.Schema and constant updates
LogsV2ResourceColumnconstant to support resource-level field resolution in log queries.Query building improvements
getKeySelectorsinstatement_builder.goto include selectors fromSelectFields, ensuring all relevant field keys are extracted for query construction. [1] [2]Testing improvements
Metrics field mapper alignment
keysparameter inColumnExpressionFor.✅ Changes
🏷️ Required: Add Relevant Labels
ex:
frontendbackenddevopsbugenhancementuitest👥 Reviewers
🧪 How to Test
🔍 Related Issues
Closes #
📸 Screenshots / Screen Recording (if applicable / mandatory for UI related changes)
📋 Checklist
👀 Notes for Reviewers
Note
Strengthens field resolution and SQL expression building, reducing ambiguity and improving query robustness.
FieldFor,ColumnFor,ColumnExpressionForto logsfield_mapperwith better errors and aliasing; buildsmultiIfacross contexts/materialized fields; introducespopulateMissingFieldContextAndDataTypeLogsV2ResourceColumn(resource) for resource-level lookups in logsSelectFieldsingetKeySelectorsinstatement_builder.goto ensure all referenced keys are fetchedColumnExpressionForsignature to acceptkeys(ignored), keeping API parityGetKeyTextFromFieldKeyintelemetrytypes/field.goWritten by Cursor Bugbot for commit f6d8ccf. This will update automatically on new commits. Configure here.