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

Add support for unassociated streams #231

Conversation

idastambuk
Copy link
Contributor

@idastambuk idastambuk commented Oct 13, 2023

What this PR does / why we need it:
When querying by propertyAlias was first implemented, it had support for unassociated streams ("disassociated" in AWS).
Unassociated streams are data streams that are not associated with a propertyId on an asset, but contain historical data about a property all the same. They can be queried by a propertyAlias.
You can see them here in the console if you apply the Disassociated Streams filter:

Screenshot 2023-10-13 at 7 13 43 PM

The 500 error from the bug report happened because of a null pointer reference since, when we query DescribeAssetPropertyWithContext for unassociated propertyAliases, it doesn't return an assetId or propertyId. The bug was introduced when we implemented fetching assets and property values only by propertyAlias. However, this only worked for associated streams, where DescribeAssetPropertyWithContext will actually return propertyId.

func getAssetIdAndPropertyId(query models.AssetPropertyValueQuery, client client.SitewiseClient, ctx context.Context) (models.AssetPropertyValueQuery, error) {
	result := query
	if query.PropertyAlias != "" {
		resp, err := client.DescribeTimeSeriesWithContext(ctx, &iotsitewise.DescribeTimeSeriesInput{
			Alias: getPropertyAlias(query.BaseQuery),
		})
		if err != nil {
			return models.AssetPropertyValueQuery{}, err
		}
		---> result.AssetIds = []string{*resp.AssetId}
		result.PropertyId = *resp.PropertyId
		
Screenshot 2023-10-16 at 12 13 14 PM

On the frontend, I fixed the panel header to show propertyAlias in the panel header when collapsed
Screenshot 2023-10-16 at 12 11 40 PM

I also moved some functions in the util package from /api, since I needed them in other packages

Which issue(s) this PR fixes:

How to test:

Add the following propertyAliased into the property alias field and test different "Get value ..." query types. For data, select "last year" in order to be able to see data, then you can zoom in.

We have a few aliased streams in our dev account:
turbine/1/torque -> disassociated - doesn't have an assetId/propertyId tied to it, so this can be tested for a fix
/turbine/1/wind_direction -> associated - for comparison

Fixes #

Special notes for your reviewer:

@idastambuk idastambuk force-pushed the 228-error-500-run-query-on-sitewise-property-value-history-for-un-associated-sitewise-data-streams branch from 6867dd9 to a8426d1 Compare October 16, 2023 01:47
@idastambuk idastambuk marked this pull request as ready for review October 16, 2023 10:14
@idastambuk idastambuk requested a review from a team as a code owner October 16, 2023 10:14
@idastambuk idastambuk requested review from fridgepoet, iwysiu and kevinwcyu and removed request for a team October 16, 2023 10:14
@fridgepoet
Copy link
Member

fridgepoet commented Oct 17, 2023

Heya @idastambuk just trying to understand what is expected here. I've tried "Get property value history" on some dissociated data streams in our squad dev account, but I'm getting a panic in the backend code still. This seems to be the same as what's currently on v1.11.1
I think it's panicking here as h is empty https://github.com/grafana/iot-sitewise-datasource/pull/231/files#diff-670b06752c76d1a93e8f926fe7568b7de2bce83536b701c5de5d723a94e4676eR53
Is that expected?
Let me know if I've got something wrong or missed something!

ex:
Screenshot 2023-10-17 at 18 08 53

pkg/framer/property_value.go Outdated Show resolved Hide resolved
pkg/server/test/property_value_aggregate_test.go Outdated Show resolved Hide resolved
@idastambuk
Copy link
Contributor Author

Heya @idastambuk just trying to understand what is expected here. I've tried "Get property value history" on some dissociated data streams in our squad dev account, but I'm getting a panic in the backend code still. This seems to be the same as what's currently on v1.11.1 I think it's panicking here as h is empty https://github.com/grafana/iot-sitewise-datasource/pull/231/files#diff-670b06752c76d1a93e8f926fe7568b7de2bce83536b701c5de5d723a94e4676eR53 Is that expected? Let me know if I've got something wrong or missed something!

ex: Screenshot 2023-10-17 at 18 08 53

Hey @fridgepoet, good catch, it only happens with property value history queries and when there is no data. I added a condition and updated the tests.

@idastambuk idastambuk merged commit 3289943 into main Oct 24, 2023
4 checks passed
@idastambuk idastambuk deleted the 228-error-500-run-query-on-sitewise-property-value-history-for-un-associated-sitewise-data-streams branch October 24, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error 500 run query on SiteWise property value history for un-associated SiteWise data streams
4 participants