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

Data: Adds support for the wide data frame format #318

Closed
wants to merge 2 commits into from

Conversation

hugohaggmark
Copy link

@hugohaggmark hugohaggmark commented Jun 22, 2021

This PR is an intermediate solution for grafana/grafana#35599 until we have a better, and more generic solution in place.

Fixes #grafana/grafana#35599

@hugohaggmark hugohaggmark self-assigned this Jun 22, 2021
@CLAassistant
Copy link

CLAassistant commented Jun 22, 2021

CLA assistant check
All committers have signed the CLA.

meta: any;
}

export function fromWideDataFrame(data: LegacyTable[]): LegacyTimeSeries[] {
Copy link
Member

Choose a reason for hiding this comment

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

thought this was going to be part of toLegacyResponseData https://github.com/grafana/grafana/blob/main/packages/grafana-data/src/dataframe/processDataFrame.ts#L320 ? Having it here means this problem is still there for other panels. But I guess it's because your waiting on grafana/grafana-plugin-sdk-go#375

Copy link
Author

Choose a reason for hiding this comment

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

yes, you're correct!

Comment on lines 244 to 250
export function containsWideDataFrame(data: any[]) {
if (!data || !data.length) {
return false;
}

return data.some((d) => d && d.hasOwnProperty('type') && d.type === 'table');
}
Copy link
Member

Choose a reason for hiding this comment

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

this check seems to only check if it's a table, is this enough?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure but I could break this out to a separate function and add more logic like fields.length > 2 I guess?

@ryantxu
Copy link
Member

ryantxu commented Jun 24, 2021

This has a ton of formatting changes that are not required (or applied automatically) by the plugin settings -- can we either exclude the formatting changes, or split this into two PRs -- one for formatting and another for the wide support. I am kinda reluctant to change the supported formats here -- this plugin is super fragile anyone trying anything too complicated should use https://github.com/panodata/grafana-map-panel

@hugohaggmark
Copy link
Author

This has a ton of formatting changes that are not required (or applied automatically) by the plugin settings -- can we either exclude the formatting changes, or split this into two PRs -- one for formatting and another for the wide support. I am kinda reluctant to change the supported formats here -- this plugin is super fragile anyone trying anything too complicated should use https://github.com/panodata/grafana-map-panel

Sure, valid point. I'll revert the formatting changes.

@hugohaggmark
Copy link
Author

@ryantxu removed all the formatting

@ryantxu
Copy link
Member

ryantxu commented Jul 14, 2021

With grafana/grafana#36737 I don't think this is still necessary

@hugohaggmark
Copy link
Author

Closing this in favor of grafana/grafana#36737

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.

4 participants