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

Azuremonitor: do not set unit if literal "Unspecified" #26839

Merged
merged 1 commit into from Aug 6, 2020

Conversation

@kylebrandt
Copy link
Contributor

@kylebrandt kylebrandt commented Aug 6, 2020

When the unit returned from azure is Unspecified, then we do not add that to frame.

part of #26828

part of #26828
@kylebrandt kylebrandt added this to the 7.1.4 milestone Aug 6, 2020
@kylebrandt kylebrandt requested a review from ryantxu Aug 6, 2020
@kylebrandt kylebrandt requested a review from grafana/backend-platform as a code owner Aug 6, 2020
@kylebrandt kylebrandt requested review from wbrowne and papagian and removed request for grafana/backend-platform Aug 6, 2020
@ryantxu
ryantxu approved these changes Aug 6, 2020
Copy link
Member

@ryantxu ryantxu left a comment

Maybe better is to have "Unspecified" return an empty string from toGrafanaUnit()?

@kylebrandt
Copy link
Contributor Author

@kylebrandt kylebrandt commented Aug 6, 2020

Maybe better is to have "Unspecified" return an empty string from toGrafanaUnit()?

I considered that, but does add a unit with a value of an empty string. While that property won't get sent to the frontend because of the json omitempty on the struct, I felt it was clearer to show that in that case we are not setting the unit.

@kylebrandt kylebrandt mentioned this pull request Aug 6, 2020
dataField.SetConfig(&data.FieldConfig{
Unit: toGrafanaUnit(amr.Value[0].Unit),
})
}

This comment has been minimized.

@papagian

papagian Aug 6, 2020
Contributor

in addition to this, what about adding a warning if unit is "unspecified"?

This comment has been minimized.

@kylebrandt

kylebrandt Aug 6, 2020
Author Contributor

Adding the unit is new, so I think of it more of a nice to have. So I would worry about making a thing that would be more likely to annoy the user than do anything useful. In particular since adding the unit has resulted in other issues that still need to be resolved ( #26831 )

@kylebrandt kylebrandt merged commit 62b103b into master Aug 6, 2020
2 checks passed
2 checks passed
continuous-integration/drone/pr Build is passing
Details
license/cla Contributor License Agreement is signed.
Details
@kylebrandt kylebrandt deleted the azUnitUnspecified branch Aug 6, 2020
aknuds1 added a commit that referenced this pull request Aug 20, 2020
wbrowne added a commit that referenced this pull request Aug 20, 2020
@wbrowne wbrowne mentioned this pull request Aug 20, 2020
wbrowne added a commit that referenced this pull request Aug 20, 2020
@aknuds1 aknuds1 mentioned this pull request Aug 20, 2020
marcusolsson added a commit to marcusolsson/grafana that referenced this pull request Sep 22, 2020
part of grafana#26828
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants