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: summary panel nrql query issues (Fixes #10) #20

Merged
merged 5 commits into from
Feb 19, 2020
Merged

fix: summary panel nrql query issues (Fixes #10) #20

merged 5 commits into from
Feb 19, 2020

Conversation

jbeveland27
Copy link
Contributor

  • fixes summary panel disappearing act by replacing the NrqlQuery component with NerdGraphQuery (also utilizes NerdGraphError for convenient error handling)
    • Additional Info: Repeated fetches with the same Nrql query using the NrqlQuery component fail to return data when NrqlQuery.FORMAT_TYPE.RAW is specified. I did a test with the SINCE clause continuously updating vs remaining static to determine this is an issue. It seems something in the service itself is not responding with data, so we may need to look into this more in case it impacts other Nerdpacks. Switching NrqlQuery out for NerdGraphQuery addressed the issue.
  • Tidying up styling so summary/charts don't jump as much when different processes are selected. Also fixed some spacing issues by updating Stack props.
  • Updated Icon.SIZE_TYPE to comply with warning from dev tools console.
  • Added nr1.json to root of project

@claassistantio
Copy link

claassistantio commented Feb 19, 2020

CLA assistant check
All committers have signed the CLA.

@jbeveland27 jbeveland27 linked an issue Feb 19, 2020 that may be closed by this pull request
Copy link
Contributor

@tangollama tangollama left a comment

Choose a reason for hiding this comment

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

In nerdlets/top-nerdlet/process-details.js, we should shift away from the common time picker nrql helper function and use the @newrelic/nr1-community function.

<NerdGraphQuery query={nrql}>
{({ loading, error, data }) => {
if (loading) {
return <Spinner style={{ height: '110px' }} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about just fillContainer vs. a custom style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do that. The difference is fillContainer covers the entire right panel (summary + charts) whereas this height restriction made it so the spinner covered just the summary component (i.e. less overall "flashing" when a new process is selected). I'm good to change it to fillContainer if we want to favor using the built-in props.

Copy link
Contributor

@tangollama tangollama left a comment

Choose a reason for hiding this comment

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

Good stuff overall. I'd like to see those two changes ahead of a merge.

Copy link
Contributor

@devfreddy devfreddy left a comment

Choose a reason for hiding this comment

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

As Joel mentioned, let's replace timePickerNrql with timeRangeToNrql, otherwise LGTM 👍

@jbeveland27
Copy link
Contributor Author

Looks like all the changes are present in the files now. Not sure why the 4th commit didn't come through in the conversation but ¯_(ツ)_/¯

@jbeveland27 jbeveland27 merged commit 36bb7b9 into newrelic:master Feb 19, 2020
devfreddy pushed a commit that referenced this pull request Feb 19, 2020
## [0.4.2](v0.4.1...v0.4.2) (2020-02-19)

### Bug Fixes

* summary panel nrql query issues (Fixes [#10](#10)) ([#20](#20)) ([36bb7b9](36bb7b9))
@devfreddy
Copy link
Contributor

🎉 This PR is included in version 0.4.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

summary-panel renders inconsistently
4 participants