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

Backend: Set field.Config.DisplayNameFromDS instead of frame.name #180

Merged
merged 6 commits into from
May 25, 2023

Conversation

fridgepoet
Copy link
Member

@fridgepoet fridgepoet commented May 22, 2023

What this PR does / why we need it:

The time series legend when displayed from a backend query (alerting, expressions) was duplicating.

This PR applies a change from Elasticsearch to generate field names for the value field of time series frames.

It applies the fixes from another PR in Elasticsearch to move from putting the frame name in frame.name to field.Config.DisplayNameFromDS and getting the metric name from metric.Field instead of metric.ID.

The screenshot illustrates the same label for a backend flow and frontend flow.
Screenshot 2023-05-22 at 18 48 19

Thank you @idastambuk for pointing out where to set the legend.

Which issue(s) this PR fixes:

Fixes #178

@fridgepoet fridgepoet requested a review from a team as a code owner May 22, 2023 16:49
@fridgepoet fridgepoet requested review from sarahzinger and kevinwcyu and removed request for a team May 22, 2023 16:49
@github-actions
Copy link
Contributor

Backend code coverage report for PR #180

Plugin Main PR Difference
opensearch 78.6% 78.1% -.5%

@github-actions
Copy link
Contributor

Frontend code coverage report for PR #180
No changes

@github-actions
Copy link
Contributor

github-actions bot commented May 22, 2023

Levitate is-compatible report:

🔍 Resolving @grafana/data@latest...
🔍 Resolving @grafana/ui@latest...
🔍 Resolving @grafana/runtime@latest...
🔍 Resolving @grafana/e2e-selectors@latest...

🔬 Checking compatibility between ./src/module.ts and @grafana/data@9.5.2...
✔ Found @grafana/data version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/ui@9.5.2...
✔ Found @grafana/ui version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/runtime@9.5.2...
✔ Found @grafana/runtime version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/e2e-selectors@9.5.2...
✔ Found @grafana/e2e-selectors version 9.0.2 locally

✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/e2e-selectors

Copy link
Member

@sarahzinger sarahzinger left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me! Good Catch!

bucket := utils.NewJsonFromAny(v)
value := castToNullFloat(bucket.Get("doc_count"))
Copy link
Member

Choose a reason for hiding this comment

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

is there any diff between castToNullFloat and castToFloat?

Copy link
Member Author

Choose a reason for hiding this comment

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

The originally-used castToNullFloat function below uses an underlying standard library type sql.NullFloat64 which is also a way to distinguish between something "null/nil" or with a float64 value. In this case, we can also just use a *float64 type to distinguish between "nil" and float64 and there is no strong reason to use an imported type for that.

Here's a similar discussion: https://stackoverflow.com/questions/33458439/using-sql-nullfloat64-vs-float64-in-golang

In Elasticsearch I noticed they moved towards *float64 so I copied that; I think that in further refactors the castToNullFloat will no longer be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backend: Time series legend is duplicating
3 participants