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

fix: use non-batch APIs at the edge #281

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

tracy-french
Copy link
Collaborator

@tracy-french tracy-french commented Jan 24, 2024

What this PR does / why we need it: IoT SiteWise BatchGet* APIs are not available at the edge. This change utilizes the non-batch variants of the APIs when at the edge.

Which issue(s) this PR fixes: #161

Copy link
Contributor

@njvrzm njvrzm left a comment

Choose a reason for hiding this comment

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

My many comments are mostly stylistic, but one or two more substantial things.

Overall I think the approach looks reasonable!

pkg/framer/property_aggregate_batch.go Outdated Show resolved Hide resolved
pkg/framer/property_aggregate_batch.go Outdated Show resolved Hide resolved
pkg/framer/property_aggregate.go Outdated Show resolved Hide resolved
pkg/framer/property_aggregate.go Outdated Show resolved Hide resolved
pkg/framer/property_aggregate.go Show resolved Hide resolved
pkg/framer/property_value_history_batch.go Show resolved Hide resolved
pkg/sitewise/api/property_aggregate_batch.go Show resolved Hide resolved
pkg/sitewise/api/property_history_batch.go Show resolved Hide resolved
pkg/sitewise/api/property_history_batch.go Show resolved Hide resolved
pkg/sitewise/api/property_value_batch.go Show resolved Hide resolved
@tracy-french
Copy link
Collaborator Author

@njvrzm Thank you for the early review!

I noticed most of your feedback is on the *_batch.go files, which is not new code. Those files are simply renamed to distinguish from the non-batch API variants. Do we need to address the issues you identified for the existing code in this PR?

@njvrzm
Copy link
Contributor

njvrzm commented Jan 25, 2024

Indeed, my apologies, it was only towards the end of reviewing that I saw that most of the code is just moved 😅 I generally try to "leave it better" but that doesn't really apply here, and no, you don't need to address most of my feedback in this PR. I'll resolve some of my comments and maybe come back to them later myself.

@tracy-french
Copy link
Collaborator Author

No worries! Thank you for clarifying!

@idastambuk
Copy link
Contributor

Hi @tracy-french, I think it makes sense to rename the files, but it could create problems down the line when we lose their entire source control history. Would it be possible for you to redo this PR in a way that preserves those files' history by renaming them with git mv?

@tracy-french
Copy link
Collaborator Author

@idastambuk For sure! I'll make that change.

@tracy-french
Copy link
Collaborator Author

@idastambuk I renamed the files with git mv, yet it didn't appear to make any difference from what I had before. Is it supposed to preserve the blame? Reading through https://stackoverflow.com/questions/1094269/whats-the-purpose-of-git-mv makes it seem like it's just a convenience function and git doesn't handle renames using it differently than a renames done outside of git?

@tracy-french tracy-french marked this pull request as ready for review January 26, 2024 17:34
@tracy-french tracy-french requested a review from a team as a code owner January 26, 2024 17:34
@tracy-french tracy-french requested review from idastambuk and kevinwcyu and removed request for a team January 26, 2024 17:34
@njvrzm
Copy link
Contributor

njvrzm commented Jan 29, 2024

Hey @tracy-french, thanks for addressing the review feedback so far. I need to figure out how to trigger the drone build so we can make sure that check runs nm, figured it out and approved the build. Meanwhile I ran the tests locally and the tests in TestPropertyValueAggregate are failing - could you fix that please?

valueField,
qualityField)

frame := data.NewFrame(*property.AssetName, timeField, valueField, qualityField)
Copy link
Contributor

@idastambuk idastambuk Jan 29, 2024

Choose a reason for hiding this comment

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

Should we call getFrameName() here instead of *property.AssetName? I remember fixing this for "raw" queries here, I assume it also applies for non-batch queries

@idastambuk
Copy link
Contributor

@idastambuk I renamed the files with git mv, yet it didn't appear to make any difference from what I had before. Is it supposed to preserve the blame? Reading through https://stackoverflow.com/questions/1094269/whats-the-purpose-of-git-mv makes it seem like it's just a convenience function and git doesn't handle renames using it differently than a renames done outside of git?

Hi @tracy-french, yes, I noticed the same, I think it might be because the original files were renamed, but they were replaced with files with the same name (property_values_non_batch.go became the original property_values.go). I overlooked that when I suggested the solution, sorry about that!

@tracy-french
Copy link
Collaborator Author

@idastambuk @njvrzm Feedback addressed! Thank you!

@idastambuk
Copy link
Contributor

Hi @tracy-french it looks like backend tests are failing, can you take a look? If you need help running them locally, let us know!

@tracy-french
Copy link
Collaborator Author

@idastambuk Sorry about that! It looks like a forgot a file when creating the commit. Working on resolving now!

@tracy-french
Copy link
Collaborator Author

@idastambuk @njvrzm Could we please merge this change in? Thank you! :)

@sarahzinger sarahzinger merged commit 6a26330 into grafana:main Feb 21, 2024
3 checks passed
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.

4 participants