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

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

part of #26828

@kylebrandt kylebrandt added this to the 7.1.4 milestone Aug 6, 2020
@kylebrandt kylebrandt requested a review from ryantxu August 6, 2020 17:36
@kylebrandt kylebrandt requested a review from a team as a code owner August 6, 2020 17:36
@kylebrandt kylebrandt requested review from wbrowne and papagian and removed request for a team August 6, 2020 17:36
Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

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

@kylebrandt
Copy link
Contributor Author

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),
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
@kylebrandt kylebrandt deleted the azUnitUnspecified branch August 6, 2020 20:03
wbrowne pushed a commit that referenced this pull request Aug 20, 2020
@wbrowne wbrowne mentioned this pull request Aug 20, 2020
wbrowne pushed a commit that referenced this pull request Aug 20, 2020
@aknuds1 aknuds1 mentioned this pull request Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog datasource/Azure Azure Monitor Datasource
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants