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

DataFrame: Align frame (__series.name) and field naming (__field.name) #69621

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Jun 6, 2023

There is a big mismatch in how we handle getFrameDisplayName and getFieldDisplayName. The problem is pretty big because we use getFrameDisplayName to evaluate ${__series.name} (which is a macro we do show first in data link auto-complete menu).

The current logic for getFrameDisplayName results in a very different name compared to getFieldDisplayName (Which we use almost everywhere else for the value field of a single time series frame).

So for time series with 2 fields (time, value) and the value field has say a displayName or displayNameFromDS ${__series.name} will currently just result in Series (<refId>) which is pretty far from the display name actually used in all visualizations (which is the getFieldDisplayName for the value field).

Changes

  • Update getFrameDisplayName to return the result of the getFieldDisplayName for DataFrames with only a single value field.

This could be a pretty big breaking change as it changes frame display names. not really sure where we use frame display names where it could cause a breaking change (other than in data links that use ${__series.name} and are counting on the bad current behavior.

Escalation where I discovered this
https://github.com/grafana/support-escalations/issues/6199

Release notice breaking change

The implementation for template macro ${__series.name} was not always correct, resulting in an interpolation that was very different from the series name displayed in the visualization. We have now fixed this issue so that it does show the same name. Depending on how ${__series.name} is used this could result in a minor breaking change.

@torkelo torkelo requested review from a team as code owners June 6, 2023 09:58
@torkelo torkelo requested review from tskarhed, ashharrison90, academo and bohandley and removed request for a team June 6, 2023 09:58
@torkelo torkelo requested review from ryantxu, dprokop and kylebrandt and removed request for tskarhed, ashharrison90 and academo June 6, 2023 09:58
Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

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

@torkelo I think this change looks good, tho I see two potential places where we need to check whether or not this is introducing some bugs:

@dprokop dprokop requested a review from ifrost June 6, 2023 10:38
@ifrost
Copy link
Contributor

ifrost commented Jun 6, 2023

Explore graph that uses getFrameDisplayName for hiding the series. It's used indirectly via Logs volume visualization. Would be good to get @ifrost eyes on this PR. Adding as a reviewer.

Thanks for tagging! Logs volume uses frame.name so this change shouldn't affect it. I tested it locally and it seems to work fine 👍 (cc @grafana/observability-logs)

@bohandley
Copy link
Contributor

This looks good to me, but does not solve the Prometheus issue where the Frame.name takes precedence over the custom legend format option when we use getFrameDisplayName. This is only in the Series to Rows transformation as far as I know, so we can still prioritize displayNameFromDS directly in that transformation if we don't want to do that here.

#69343

@torkelo torkelo added this to the 10.1.x milestone Jun 7, 2023
@torkelo torkelo added add to changelog breaking change Relevant for changelog generation no-backport Skip backport of PR labels Jun 7, 2023
@torkelo torkelo merged commit 862b04c into main Jun 8, 2023
32 checks passed
@torkelo torkelo deleted the frame-name-field-name-alignment branch June 8, 2023 07:52
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend breaking change Relevant for changelog generation no-backport Skip backport of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants