appmetrics-dash icon indicating copy to clipboard operation
appmetrics-dash copied to clipboard

better UX when node-report cannot be required

Open sam-github opened this issue 7 years ago • 22 comments

node-report is an optional dep because it will often not be possible to install (Windows and production Linux machines often don't have compilers).

This is handled like this right now, but perhaps the button itself should not be shown if its not going to work?

If part of the UI can show some basic app stats (node version, CWD, package.name), perhaps added to that could be some info about node-report status, and the button can be present or not depending on whether the feature works.

Or alternatively, when the button gets and error back, perhaps it can popup and say why the report couldn't be made, and what has to be done to fix it.

/cc @rnchamberlain @smartmouse @rgcurtis @aydonat

sam-github avatar Feb 27 '17 22:02 sam-github

I like "If part of the UI can show some basic app stats (node version, CWD, package.name), perhaps added to that could be some info about node-report status, and the button can be present or not depending on whether the feature works."

aydonat avatar Feb 27 '17 23:02 aydonat

yep this will improve usability.

smartmouse avatar Feb 28 '17 02:02 smartmouse

@aydonat How should we document this? Just add a note that says something like "Node Report is not currently supported." Or...?

crandmck avatar Mar 06 '17 21:03 crandmck

did we make the UX changes that I recommended?

aydonat avatar Mar 06 '17 21:03 aydonat

@aydonat No, you requested an entire new feature! If that feature existed, your suggestion would make sense, but you'd have to request and get that new feature triaged against other priorities.

sam-github avatar Mar 06 '17 23:03 sam-github

ok, just checking :)

then @crandmck please document "Node Report is currently not supported for such and such case."

aydonat avatar Mar 06 '17 23:03 aydonat

@sam-github Is it possible for users to get "Node Report" to work in this release? If so, how? Is it just a matter of having a compiler? (I have compiler installed, but it didn't work for me, so I'm a bit confused...)

crandmck avatar Mar 07 '17 23:03 crandmck

@crandmck Whats the problem you are seeing ? Node report is working on the dashboard when deployed outside APIC

tobespc avatar Mar 08 '17 08:03 tobespc

Most users won't have compiler tools, so it won't build /cc @rnchamberlain, who knows about this, but just so he can observe the effect.

apiconnect also strips all optional dependencies (node-report is optional) before packaging.

At least, it does before release. npm install -g apiconnect --registry apiconnect01.rchland.ibm.com:4873 doesn't have a shrinkwrap, so its not yet a release, so I can't verify, but unless something changed, no user of apiconnect/the apic CLI will have node-report.

sam-github avatar Mar 08 '17 17:03 sam-github

Btw, the reason apiconnect strips them, is because it shrinkwraps, and shrinkwrap-specified dependencies are not optional, they must succeed during install, and since compiled deps often don't and are made optional, they cannot be included in shrinkwrap. The mechanism to do this is to remove all optional deps, since finding compiled deps is much harder, and anyhow, some compiled deps are pre-compiled, and that is even harder to detect, just stripping optional deps before shrinkwrapping is more robust.

sam-github avatar Mar 08 '17 17:03 sam-github

@tobespc When I click on the Node Report button in dashboard, I get

node reporting not available

In Slack, Sam said:

one of two things happened:

  1. because of apiconnect's release process, there is a shrinkwrap somewhere that prevents node-report from being installed (it won't even be attempted)
  2. there was a failed to compile message, and you missed it

I'm trying to reinstall to see if there are any failures, but running into other issues w/internal registry.

crandmck avatar Mar 08 '17 18:03 crandmck

ok, so that means the node-report has not been built as part of the installation when using apiconnect release process. I've not seen that process, is it something you can point me at so I can try ?

tobespc avatar Mar 09 '17 08:03 tobespc

@aydonat ^?

crandmck avatar Mar 09 '17 16:03 crandmck

@sam-github might be able to help

aydonat avatar Mar 09 '17 18:03 aydonat

@tobespc: I can give you an environment to check. I discovered this issue on windows but we currently have build issues. will update when we build is resolved.

smartmouse avatar Mar 09 '17 21:03 smartmouse

I think I see that issue @sam-github sam meant

Logs Gateway LogsApplication Logs

This is on windows -- but starting from designer.

[03/09/17 22:12:19] com.ibm.diagnostics.healthcenter.loader INFO: Node Application Metrics 1.2.0.201612012154 (Agent Core 3.1.0)  
[03/09/17 22:12:19] com.ibm.diagnostics.healthcenter.mqtt INFO: Connecting to broker localhost:1883  
strong-supervisor attaching dashboard at /appmetrics-dash 
2017-03-10T03:12:19.917Z pid:6468 worker:0 INFO supervisor starting (pid 6468) 
2017-03-10T03:12:19.922Z pid:6468 worker:0 INFO supervisor reporting metrics to `internal:` 
2017-03-10T03:12:19.952Z pid:6468 worker:0 INFO supervisor size set to undefined 
2017-03-10T03:12:21.393Z pid:4496 worker:1 [03/09/17 22:12:21] com.ibm.diagnostics.healthcenter.loader INFO: Node Application Metrics 1.2.0.201612012154 (Agent Core 3.1.0) 
2017-03-10T03:12:21.518Z pid:4496 worker:1 [03/09/17 22:12:21] com.ibm.diagnostics.healthcenter.mqtt INFO: Connecting to broker localhost:1883 
2017-03-10T03:12:21.568Z pid:4496 worker:1 strong-supervisor attaching dashboard at /appmetrics-dash 
2017-03-10T03:12:26.050Z pid:4496 worker:1 Cannot find module 'node-report/api': skipping optional dependency 
2017-03-10T03:12:26.284Z pid:4496 worker:1 Web server listening at: http://localhost:4003 

screen shot 2017-03-09 at 10 27 28 pm screen shot 2017-03-09 at 10 33 24 pm

but this machine does not have a compiler and I am using the bundle build.

node 4.4.x npm 4.x

smartmouse avatar Mar 10 '17 03:03 smartmouse

I think I see the problem, in the screenshot above you are not using the appmetrics that has node-report support in

appmetrics 1.2.0.201612012154

you need to be on 2.0.1 which is the latest in npm

tobespc avatar Mar 10 '17 09:03 tobespc

I am using the master apiconnect bundle build that is available yesterday but this issue only occur on windows. It works on other OS.

smartmouse avatar Mar 10 '17 14:03 smartmouse

I am updating strong-supervisor to appmetrics@2, thanks for pointing that out.

node-report isn't builtin to any version of appmetrics, AFAICT

sam-github avatar Mar 10 '17 16:03 sam-github

ok, so that means the node-report has not been built as part of the installation when using apiconnect release process. I've not seen that process, is it something you can point me at so I can try ?

I am looking into this Toby, so you don't have to, but to answer your question if you want to be more familiar with apiconnect packaging:

go to https://apim-jenkins.hursley.ibm.com/view/All/job/apiconnect-bundles/2318/ <--- download apiconnect-2.6.0.tgz and do npm i -g /path/to/apiconnect-2.6.0.tgz. that is the .tgz that is installed by apiconnect users.

sam-github avatar Mar 10 '17 16:03 sam-github

@NikCanvin @tobespc I think this issue comment thread went sideways. The issue isn't whether node-report will be available, its what the UX will be when it is not available. The current UX is good enough for a first release right now, but it could stand some improvement.

That node-report will never be available for APIConnect is a fact of their release process, see below. It will also not be available for any other users of the appmetrics-dash if they don't have compiler tools (common on Windows and OS X, which covers almost 100% of developer laptops and work stations). Most production deploys are on Linux, but even Linux production machines aren't guaranteed to have compiler tools. Its poor security practice to have compilers on production machines, though they do tend to be there on many default Linux installs. This is why its so great that appmetrics is pre-compiled for all supported platforms and requires no network access at install time. node-report may also be like that eventually, but the community build infrastructure changes have not yet been made, and @rnchamberlain is aware of the issue.

I have confirmed that node-report will not be installed or available when using apiconnect, ever, even if compiler tools are installed on the platform. /to @aydonat @crandmck @smartmouse

Doing an install as described in https://github.com/RuntimeTools/appmetrics-dash/issues/26#issuecomment-285712362 using nvm and node 4.8.0 (not that use of nvm or node version is in any way relevant to this!):

lib/node_modules(:589a911|✔) % npm ls appmetrics
/Users/sam/.nvm/versions/node/v4.8.0/lib
└─┬ [email protected]
  ├── [email protected] 
  └─┬ [email protected]
    └── [email protected] 

lib/node_modules(:589a911|✔) % npm ls strong-supervisor
/Users/sam/.nvm/versions/node/v4.8.0/lib
└─┬ [email protected]
  └── [email protected] 

lib/node_modules(:589a911|✔) % npm ls node-report      
/Users/sam/.nvm/versions/node/v4.8.0/lib
└── (empty)

npm ERR! code 1
lib/node_modules(:589a911|✔) % pwd
/Users/sam/.nvm/versions/node/v4.8.0/lib/node_modules

sam-github avatar Mar 10 '17 16:03 sam-github

I have confirmed that node-report will not be installed or available when using apiconnect, ever, even if compiler tools are installed on the platform. /to @aydonat @crandmck @smartmouse

Thanks, that resolves the issue for me. I will simply state that in the docs for this release.

crandmck avatar Mar 10 '17 18:03 crandmck