fix: enrich attributes regardless if it is materialized #6000

nityanandagohain · 2024-09-18T05:41:28Z

Changes

If a column is selected and later it was removed, or the dashboard is copied to an instance where it is not selected one then we should enrich regardless so that the query doesn't fail.
Take care if the same key name is present in resource and tag or have multiple dataType.

Fixes https://github.com/SigNoz/engineering-pod/issues/1774

github-actions · 2024-09-18T05:41:44Z

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

pkg/query-service/app/logs/v3/enrich_query_test.go

github-actions · 2024-09-18T09:28:42Z

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

github-actions · 2024-09-18T09:28:48Z

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

srikanthccv · 2024-09-18T11:10:50Z

pkg/query-service/app/logs/v3/enrich_query.go

-			return existingField
+	for _, key := range generateEnrichmentKeys(field) {
+		if val, ok := fields[key]; ok {
+			return val


if field.Type != v3.AttributeKeyTypeUnspecified && field.DataType != v3.AttributeKeyDataTypeUnspecified { names = append(names, field.Key+"##"+field.Type.String()+"##"+field.DataType.String()) return names }

Assume a user has requested with type: tag and datatype: string for a key http.status_code as an example. However, we only have a number type for it in DB. With this logic, it will continue to search for strings because http.status_code##tag##string doesn't exist in generateEnrichmentKeys and the fallback is to use whatever the user provided. What is the sensible thing to do here? Reject the request saying there is no http.status_code with string?

Since we are touching the enrichment part, I am trying to highlight the problems/cases so we can have a definitive solution to enrichment and never worry about it again.

We should run the query as it is in that case and let the query fail. ( return empty results)

So we can say that enrich query

If type and dataType is provided for an attribute, it will enrich if it's materialized or dematerialized.

If one of type and dataType or both is not present, it will try to get whatever possible from the existing fields.

works correctly if there are no duplicate in attribute/resources or in dataType ( might be incorrect if there are duplicates)

So with this we can say that if user has provided, type, dataType and attribute in that case we should respect the user and just see if there is a corresponding materialized column. We would also want to do this for an attribute to arrive in the future.

We would also want to do this for an attribute to arrive in the future.

There is no contention about this case.

We should run the query as it is in that case and let the query fail. ( return empty results)

I don't agree especially because when we know it's going to fail and alternative metadata would prevent it from happening we shouldn't knowingly fail. The main problem with these kinds of issues is they are silent and can be very frustrating to troubleshoot. Would you accept this if you were at the receiving end of the result?

You can choose not to solve it right now by making a case that the likelihood of this happening is low and that you would rather get this change unblocked, but it's an entirely different thing to say we should let the query fail.

very frustrating to troubleshoot.

I don't think that should be the case, as suggestions will help the user choose the correct attribute for querying. In case of API's user might actually search for that attribute and changing it when they are actually providing something they want to use might not be a good idea according to me. What do you think ?

I don't think that should be the case, as suggestions will help the user choose the correct attribute for querying

The focus shouldn't be solely on the search bar as the source of this issue. The problem can manifest in various scenarios. Consider a situation where a user has a saved view, but the associated metadata no longer aligns with what's currently in the database. This metadata mismatch can occur due to factors beyond the user's control, such as third-party instrumentation changes or a colleague modifying data types. Why has a previously functional query stopped yielding results? How can users identify what has changed between the time when the query produced results and now when it returns nothing?

In case of API's user might actually search for that attribute and changing it when they are actually providing something they want to use might not be a good idea according to me.

Not sure if it came across that way, but I didn't mean to suggest we change it automatically without their knowledge. I was asking what would be the sensible thing to do in such cases here: #6000 (comment)

Got your point.

So if user has provided a type and datatype and it doesn't exist, fallback to whatever is possible from the fields map is it ?

I don't have a (preferred) solution. I am just thinking out loud and highlighting where the enrichment logic breaks and leads to poor user experience. It doesn't even have to be addressed in this PR, but listing out where the enrichment logic breaks is important.

Agree,

Also on a side note as of now these fields that we store are permanent and not bound to time. Which means let say a user sends an attribute x with type a. It will forever remain in the db that there is an attribute x with type a, even if the type changes in future. Unless we clear the db.

So in some cases we manually go ahead and truncate the attribute_keys table so that user doesn't see old keys.

W.r.t respect to the above logic I the current implementation should be fine, since once a attribute is ingested it will remain in context forever. What do you think ?

srikanthccv · 2024-09-19T03:29:50Z

pkg/query-service/model/logs.go

@@ -49,7 +49,8 @@ func GetLogFieldsV3(ctx context.Context, queryRangeParams *v3.QueryRangeParamsV3
 				if pass {
 					continue
 				}
-				data[selectedField.Name] = v3.AttributeKey{
+				name := selectedField.Name + "##" + fieldType.String() + "##" + selectedField.DataType
+				data[name] = v3.AttributeKey{


The change in the key format needs to be handled wherever GetLogFieldsV3 is used. A quick search says it's used to create links. Please check.

oh right, while all others are using it for enrichment, metaForLinks is using it differently.

Thanks have made the changes for this. I didn't past the alertType as passing it results in cyclic dependency so went ahead with a boolean flag.

fix: enrich attributes regardless if it is materialized

da712cc

nityanandagohain requested a review from srikanthccv September 18, 2024 05:41

github-actions bot added the bug Something isn't working label Sep 18, 2024

srikanthccv reviewed Sep 18, 2024

View reviewed changes

pkg/query-service/app/logs/v3/enrich_query_test.go Outdated Show resolved Hide resolved

feat: take care of same key name with different type and datatype

d8271ad

srikanthccv reviewed Sep 18, 2024

View reviewed changes

srikanthccv reviewed Sep 19, 2024

View reviewed changes

nityanandagohain added 2 commits September 19, 2024 16:08

fix: makeLinks updated with new logic for logs

142c60f

Merge remote-tracking branch 'origin/develop' into issue_1774

9819808

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enrich attributes regardless if it is materialized #6000

fix: enrich attributes regardless if it is materialized #6000

nityanandagohain commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

srikanthccv Sep 18, 2024

srikanthccv Sep 18, 2024

nityanandagohain Sep 18, 2024 •

edited

Loading

srikanthccv Sep 19, 2024

nityanandagohain Sep 19, 2024 •

edited

Loading

srikanthccv Sep 19, 2024

nityanandagohain Sep 19, 2024 •

edited

Loading

srikanthccv Sep 19, 2024 •

edited

Loading

nityanandagohain Sep 19, 2024

nityanandagohain Sep 19, 2024

srikanthccv Sep 19, 2024

nityanandagohain Sep 19, 2024

nityanandagohain Sep 19, 2024

fix: enrich attributes regardless if it is materialized #6000

Are you sure you want to change the base?

fix: enrich attributes regardless if it is materialized #6000

Conversation

nityanandagohain commented Sep 18, 2024 • edited Loading

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nityanandagohain Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nityanandagohain Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nityanandagohain Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

srikanthccv Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nityanandagohain commented Sep 18, 2024 •

edited

Loading

nityanandagohain Sep 18, 2024 •

edited

Loading

nityanandagohain Sep 19, 2024 •

edited

Loading

nityanandagohain Sep 19, 2024 •

edited

Loading

srikanthccv Sep 19, 2024 •

edited

Loading