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

NodeGraph: Fix edges dataframe miscategorization #76842

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Oct 19, 2023

A dataframe named "edges" could end up getting categorized as a nodes dataframe if it was missing a "source" field, resulting in a very confusing error message "id field is required for nodes dataframe" instead of a more sensible error message about the missing source field.

A dataframe named "edges" could end up getting categorized as a nodes dataframe if it was missing a "source" field, resulting in a very confusing error message "id field is required for nodes dataframe" instead of a more sensible error message about the missing source field.
@lovasoa lovasoa requested a review from a team as a code owner October 19, 2023 23:33
@CLAassistant
Copy link

CLAassistant commented Oct 19, 2023

CLA assistant check
All committers have signed the CLA.

@grafana-pr-automation grafana-pr-automation bot added area/frontend pr/external This PR is from external contributor labels Oct 19, 2023
@lovasoa
Copy link
Contributor Author

lovasoa commented Oct 19, 2023

Additionally, I'd like to suggest that "display names" of fields are considered when looking up columns instead of just "source names".

@aocenas
Copy link
Member

aocenas commented Oct 20, 2023

I wonder if we should be a bit more smart with the categorization. There are few required columns for both the node and edges data frames and maybe we could just check them thoroughly to make sure we don't pass on an invalid data frame. Would you have time to do that? If not we can merge this PR and just leave it for later.

Additionally, I'd like to suggest that "display names" of fields are considered when looking up columns instead of just "source names".

This probably would not work well. The display names are to give columns nicer names when presented to the user. So you can for example label source/target as Client/Server for example. Because we want to give users the liberty to use any labels they want we cannot use the same thing to structurally match columns. If you are mentioning this because you cannot use rename transformations to match you data to the structure needed for the nodeGraph, we have an open PR right now to allow mapping different column names in a panel config.

@aocenas aocenas self-requested a review October 20, 2023 08:37
@aocenas aocenas self-assigned this Oct 20, 2023
@aocenas aocenas changed the title in node graph, fix edges dataframe miscategorization NodeGraph: Fix edges dataframe miscategorization Oct 20, 2023
@lovasoa
Copy link
Contributor Author

lovasoa commented Oct 20, 2023

Would you have time to do that?

That was a quick test I was doing, I don't really have the time to hack on grafana at the moment. SQLPage is keeping me busy. And my day job, also ;)

If you are mentioning this because you cannot use rename transformations to match you data to the structure needed for the nodeGraph, we have an open PR right now to allow mapping different column names in a panel config.

Yes, that was my problem, and mapping column names in the config would solve it even better 🙂

@aocenas aocenas added this to the 10.2.x milestone Oct 20, 2023
@aocenas
Copy link
Member

aocenas commented Oct 20, 2023

here is the mapping pr btw #76009

Also I think this look good, will wait for the checks to pass and merge this.

@aocenas aocenas added no-backport Skip backport of PR and removed no-backport Skip backport of PR labels Oct 20, 2023
@aocenas aocenas merged commit 4b6b3b7 into grafana:main Oct 23, 2023
14 checks passed
@lovasoa
Copy link
Contributor Author

lovasoa commented Oct 23, 2023

Thanks !

@zerok zerok modified the milestones: 10.2.x, 10.2.0, 10.3.x Oct 23, 2023
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants