Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: use an equivalent fixed column for tag attributes #5028

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft

chore: use an equivalent fixed column for tag attributes #5028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9c8e750
chore: ignore tag attributes which has an equivalent fixed column
makeavish May 20, 2024
5d8c2f7
Merge branch 'develop' into chore/ignore-duplicate-tag-keys
makeavish May 21, 2024
d05d906
Merge branch 'develop' into chore/ignore-duplicate-tag-keys
makeavish May 24, 2024
74e8d22
Merge branch 'develop' into chore/ignore-duplicate-tag-keys
makeavish May 24, 2024
3170c34
chore: use equivalent column attribute
makeavish May 24, 2024
12223e0
Merge branch 'develop' into chore/ignore-duplicate-tag-keys
makeavish May 24, 2024
caef0ae
Merge branch 'develop' into chore/ignore-duplicate-tag-keys
makeavish Jun 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/query-service/app/clickhouseReader/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4833,6 +4833,7 @@ func (r *ClickHouseReader) GetTraceAggregateAttributes(ctx context.Context, req
if err := rows.Scan(&tagKey, &tagType, &dataType, &isColumn); err != nil {
return nil, fmt.Errorf("error while scanning rows: %s", err.Error())
}

// TODO: Remove this once the column name are updated in the table
tagKey = tempHandleFixedColumns(tagKey)
key := v3.AttributeKey{
Expand Down Expand Up @@ -4874,6 +4875,7 @@ func (r *ClickHouseReader) GetTraceAttributeKeys(ctx context.Context, req *v3.Fi
if err := rows.Scan(&tagKey, &tagType, &dataType, &isColumn); err != nil {
return nil, fmt.Errorf("error while scanning rows: %s", err.Error())
}

// TODO: Remove this once the column name are updated in the table
tagKey = tempHandleFixedColumns(tagKey)
key := v3.AttributeKey{
Expand Down
142 changes: 142 additions & 0 deletions pkg/query-service/app/http_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2999,6 +2999,148 @@ func (aH *APIHandler) getSpanKeysV3(ctx context.Context, queryRangeParams *v3.Qu
Key: "timestamp",
IsColumn: true,
}
spanKeys["http.url"] = v3.AttributeKey{
Key: "httpUrl",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["db.system"] = v3.AttributeKey{
Key: "dbSystem",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["db.name"] = v3.AttributeKey{
Key: "dbName",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["db.operation"] = v3.AttributeKey{
Key: "dbOperation",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["peer.service"] = v3.AttributeKey{
Key: "peerService",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["url.full"] = v3.AttributeKey{
Key: "httpUrl",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["http.response.status_code"] = v3.AttributeKey{
Key: "responseStatusCode",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["http.route"] = v3.AttributeKey{
Key: "httpRoute",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["http.method"] = v3.AttributeKey{
Key: "httpMethod",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["http.request.method"] = v3.AttributeKey{
Key: "httpMethod",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["http.status_code"] = v3.AttributeKey{
Key: "responseStatusCode",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["http.host"] = v3.AttributeKey{
Key: "httpHost",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["http.host"] = v3.AttributeKey{
Key: "httpHost",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["http.request.header.host"] = v3.AttributeKey{
Key: "httpHost",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["server.address"] = v3.AttributeKey{
Key: "httpHost",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["client.address"] = v3.AttributeKey{
Key: "httpHost",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
Comment on lines +3080 to +3097
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the equivalent change in collect is recent and not released yet, we either have to hold this PR for enough data to backfill or just comment out these new additions so users don't see incorrect results for longer time ranges.

spanKeys["messaging.system"] = v3.AttributeKey{
Key: "msgSystem",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["messaging.operation"] = v3.AttributeKey{
Key: "msgOperation",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["service.name"] = v3.AttributeKey{
Key: "serviceName",
IsColumn: true,
}
spanKeys["rpc.method"] = v3.AttributeKey{
Key: "rpcMethod",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["rpc.service"] = v3.AttributeKey{
Key: "rpcService",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["rpc.system"] = v3.AttributeKey{
Key: "rpcSystem",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["rpc.jsonrpc.error_code"] = v3.AttributeKey{
Key: "responseStatusCode",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
spanKeys["rpc.grpc.status_code"] = v3.AttributeKey{
Key: "responseStatusCode",
Type: v3.AttributeKeyTypeTag,
IsColumn: true,
DataType: v3.AttributeKeyDataTypeString,
}
return spanKeys, nil
}
}
Expand Down
23 changes: 11 additions & 12 deletions pkg/query-service/app/traces/v3/query_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,18 +85,17 @@ func getClickhouseTracesColumnDataTypeAndType(key v3.AttributeKey) (v3.Attribute
}

func enrichKeyWithMetadata(key v3.AttributeKey, keys map[string]v3.AttributeKey) v3.AttributeKey {
if key.Type == "" || key.DataType == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what breaks with removing this is when there is an attribute different type or datatype, irrespective of what user explicitly selects their preferred type and datatype, we override? I am not sure how much of a real problem this is. I have come across any customer who had the same attribute name with a different type or data type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be an issue but only when user is using custom attribute key with same name as otel key. I haven't encountered any users with the same attribute name and different types.
In logs too we are also overriding them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be an issue but only when user is using custom attribute key with same name as otel key. I haven't encountered any users with the same attribute name and different types. In logs too we are also overriding them.

There are a few users with the same key but different type/data type and they are not necessarily because of the custom attribute collision with otel key. See #5150 & #5359; @nityanandagohain created the issue after a bug request from a customer and then oss user also added that they are impacted by it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be an issue but only when user is using custom attribute key with same name as otel key. I haven't encountered any users with the same attribute name and different types. In logs too we are also overriding them.

There are a few users with the same key but different type/data type and they are not necessarily because of the custom attribute collision with otel key. See #5150 & #5359; @nityanandagohain created the issue after a bug request from a customer and then oss user also added that they are impacted by it.

Yeah, I saw community user faced the issue.

Let's block this until we find a fix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are independent problems, aren't they? Could we modify this pull request to still honor the user's selection (assuming the request payload is accurate), but for the attributes we know have fixed columns, use those instead?

// check if the key is present in the keys map
if existingKey, ok := keys[key.Key]; ok {
key.IsColumn = existingKey.IsColumn
key.Type = existingKey.Type
key.DataType = existingKey.DataType
} else { // if not present then set the default values
key.Type = v3.AttributeKeyTypeTag
key.DataType = v3.AttributeKeyDataTypeString
key.IsColumn = false
return key
}
// check if the key is present in the keys map
if existingKey, ok := keys[key.Key]; ok {
key.Key = existingKey.Key
key.IsColumn = existingKey.IsColumn
key.Type = existingKey.Type
key.DataType = existingKey.DataType
} else if key.Type == "" || key.DataType == "" { // if not present then set the default values
key.Type = v3.AttributeKeyTypeTag
key.DataType = v3.AttributeKeyDataTypeString
key.IsColumn = false
return key
}
return key
}
Expand Down
22 changes: 11 additions & 11 deletions pkg/query-service/app/traces/v3/query_builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,12 +638,12 @@ var testBuildTracesQueryData = []struct {
BuilderQuery: &v3.BuilderQuery{
QueryName: "A",
StepInterval: 60,
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag},
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag, IsColumn: false},
AggregateOperator: v3.AggregateOperatorCountDistinct,
Expression: "A",
},
TableName: "signoz_traces.distributed_signoz_index_v2",
ExpectedQuery: "SELECT toStartOfInterval(timestamp, INTERVAL 60 SECOND) AS ts, toFloat64(count(distinct(stringTagMap['name'])))" +
ExpectedQuery: "SELECT toStartOfInterval(timestamp, INTERVAL 60 SECOND) AS ts, toFloat64(count(distinct(name)))" +
" as value from signoz_traces.distributed_signoz_index_v2 where" +
" (timestamp >= '1680066360726210000' AND timestamp <= '1680066458000000000') group by ts order by value DESC",
PanelType: v3.PanelTypeGraph,
Expand Down Expand Up @@ -922,7 +922,7 @@ var testBuildTracesQueryData = []struct {
BuilderQuery: &v3.BuilderQuery{
QueryName: "A",
StepInterval: 60,
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag},
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag, IsColumn: false},
AggregateOperator: v3.AggregateOperatorCountDistinct,
Expression: "A",
Having: []v3.Having{
Expand All @@ -934,7 +934,7 @@ var testBuildTracesQueryData = []struct {
},
},
TableName: "signoz_traces.distributed_signoz_index_v2",
ExpectedQuery: "SELECT toStartOfInterval(timestamp, INTERVAL 60 SECOND) AS ts, toFloat64(count(distinct(stringTagMap['name']))) as value" +
ExpectedQuery: "SELECT toStartOfInterval(timestamp, INTERVAL 60 SECOND) AS ts, toFloat64(count(distinct(name))) as value" +
" from signoz_traces.distributed_signoz_index_v2 where (timestamp >= '1680066360726210000' AND timestamp <= '1680066458000000000')" +
" group by ts having value > 10 order by value DESC",
PanelType: v3.PanelTypeGraph,
Expand All @@ -946,7 +946,7 @@ var testBuildTracesQueryData = []struct {
BuilderQuery: &v3.BuilderQuery{
QueryName: "A",
StepInterval: 60,
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag},
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag, IsColumn: false},
AggregateOperator: v3.AggregateOperatorCount,
Expression: "A",
Filters: &v3.FilterSet{Operator: "AND", Items: []v3.FilterItem{
Expand All @@ -964,7 +964,7 @@ var testBuildTracesQueryData = []struct {
TableName: "signoz_traces.distributed_signoz_index_v2",
ExpectedQuery: "SELECT toStartOfInterval(timestamp, INTERVAL 60 SECOND) AS ts, toFloat64(count()) as value from " +
"signoz_traces.distributed_signoz_index_v2 where (timestamp >= '1680066360726210000' AND timestamp <= '1680066458000000000') " +
"AND stringTagMap['method'] = 'GET' AND has(stringTagMap, 'name') group by ts having value > 10 order by value DESC",
"AND stringTagMap['method'] = 'GET' AND name != '' group by ts having value > 10 order by value DESC",
PanelType: v3.PanelTypeGraph,
},
{
Expand All @@ -990,7 +990,7 @@ var testBuildTracesQueryData = []struct {
},
},
TableName: "signoz_traces.distributed_signoz_index_v2",
ExpectedQuery: "SELECT toStartOfInterval(timestamp, INTERVAL 60 SECOND) AS ts, toFloat64(count(distinct(stringTagMap['name']))) as value" +
ExpectedQuery: "SELECT toStartOfInterval(timestamp, INTERVAL 60 SECOND) AS ts, toFloat64(count(distinct(name))) as value" +
" from signoz_traces.distributed_signoz_index_v2 where (timestamp >= '1680066360726210000' AND timestamp <= '1680066458000000000') " +
"AND stringTagMap['method'] = 'GET' group by ts having value > 10 order by value DESC",
PanelType: v3.PanelTypeGraph,
Expand All @@ -1002,7 +1002,7 @@ var testBuildTracesQueryData = []struct {
BuilderQuery: &v3.BuilderQuery{
QueryName: "A",
StepInterval: 60,
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag},
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag, IsColumn: false},
AggregateOperator: v3.AggregateOperatorCount,
Expression: "A",
Filters: &v3.FilterSet{Operator: "AND", Items: []v3.FilterItem{
Expand All @@ -1020,7 +1020,7 @@ var testBuildTracesQueryData = []struct {
TableName: "signoz_traces.distributed_signoz_index_v2",
ExpectedQuery: "SELECT toStartOfInterval(timestamp, INTERVAL 60 SECOND) AS ts, toFloat64(count()) as value" +
" from signoz_traces.distributed_signoz_index_v2 where (timestamp >= '1680066360726210000' AND timestamp <= '1680066458000000000') " +
"AND stringTagMap['method'] = 'GET' AND has(stringTagMap, 'name') group by ts having value > 10",
"AND stringTagMap['method'] = 'GET' AND name != '' group by ts having value > 10",
PanelType: v3.PanelTypeValue,
},
{
Expand Down Expand Up @@ -1211,7 +1211,7 @@ var testPrepTracesQueryData = []struct {
GroupBy: []v3.AttributeKey{{Key: "method", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag}},
},
ExpectedQuery: "SELECT `method` from (SELECT stringTagMap['method'] as `method`," +
" toFloat64(count(distinct(stringTagMap['name']))) as value from signoz_traces.distributed_signoz_index_v2" +
" toFloat64(count(distinct(name))) as value from signoz_traces.distributed_signoz_index_v2" +
" where (timestamp >= '1680066360000000000' AND timestamp <= '1680066420000000000') AND" +
" stringTagMap['method'] = 'GET' AND has(stringTagMap, 'method') group by `method` order by value DESC) LIMIT 10",
Keys: map[string]v3.AttributeKey{"name": {Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag, IsColumn: true}},
Expand Down Expand Up @@ -1339,7 +1339,7 @@ var testPrepTracesQueryData = []struct {
End: 1680066458000,
BuilderQuery: &v3.BuilderQuery{
QueryName: "A",
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag},
AggregateAttribute: v3.AttributeKey{Key: "name", DataType: v3.AttributeKeyDataTypeString, Type: v3.AttributeKeyTypeTag, IsColumn: false},
AggregateOperator: v3.AggregateOperatorCountDistinct,
Expression: "A",
Filters: &v3.FilterSet{Operator: "AND", Items: []v3.FilterItem{
Expand Down
Loading