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.

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

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

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

Conversation

makeavish
Copy link
Member

@makeavish makeavish commented May 20, 2024

No description provided.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added bug Something isn't working chore labels May 20, 2024
@srikanthccv
Copy link
Member

How about updating the logic to only show the original attributes and updating the query builder logic to use the column in the query for those attributes which have a column. This leads to an experience that doesn't require any explanation for what is different b/w dbSystem and db.system to the user and the same experience as logs.

@makeavish
Copy link
Member Author

How about updating the logic to only show the original attributes and updating the query builder logic to use the column in the query for those attributes which have a column. This leads to an experience that doesn't require any explanation for what is different b/w dbSystem and db.system to the user and the same experience as logs.

But we want to show users which column is indexed and which is not.

@srikanthccv
Copy link
Member

srikanthccv commented May 21, 2024

You can still show that they are indexed like logs do with the same original attribute names.

@makeavish makeavish changed the title chore: ignore tag attributes which has an equivalent fixed column chore: use an equivalent fixed column for tag attributes May 24, 2024
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Minor comments

Comment on lines +3187 to +3204
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,
}
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.

@@ -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?

@makeavish makeavish removed the bug Something isn't working label Jun 29, 2024
@github-actions github-actions bot added the bug Something isn't working label Jun 29, 2024
@makeavish makeavish marked this pull request as draft July 5, 2024 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants