RuntimeTools / appmetrics-dash

A data visualizer that uses " Node Application Metrics" (appmetrics) to monitor and display Node.js application data as a html web application.
Other
452 stars 55 forks source link

Allow appsody apps to use socket.io #184

Closed sam-github closed 4 years ago

sam-github commented 4 years ago
Instead of fighting over ownership of the http request event, expose the
dashboard on the user-provided express app.

server.js is modelled after the one from the appsody nodejs-express
stack, stripped down to a minimal reproduction of the chat application
failing.

Compare to:

https://github.com/appsody/stacks/blob/dc03bec53c0bfa5/incubator/nodejs-express/image/project/server.js

-- test: chat example using pre-installed socket.io

It doesn't have to use the same socket.io version as the dashboard, but
doing so allows it to use the appmetrics-dash/node_modules, instead of
having to do a `cd test/apps/chat-example; npm i` as a pre-test step.

--

test: chat-example from socket.io

From:
- https://github.com/socketio/chat-example/tree/e2530b23ffc9c17c3a1a3

The first commits add the chat example from socket.io, and show that it can work with appmetrics-dashboard by avoiding .attach() and using .monitor().

Trying that approach with the nodejs-express stack in appsody failed, so the next commits add a reproduction of the appsody failure modelled after its server.js, and modify the monitor() API to use the root express app rather than attempting to replace the http 'request' event listener.

sam-github commented 4 years ago

Sorry, @tobespc, I fixed the example apps and repushed.

codecov-io commented 4 years ago

Codecov Report

Merging #184 into master will increase coverage by 21.81%. The diff coverage is 92.53%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #184       +/-   ##
===========================================
+ Coverage   64.42%   86.23%   +21.81%     
===========================================
  Files           4        4               
  Lines         416      356       -60     
===========================================
+ Hits          268      307       +39     
+ Misses        148       49       -99
Impacted Files Coverage Δ
lib/appmetrics-dash.js 92.14% <92.53%> (+31.22%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 00fbae2...42c656d. Read the comment docs.

sam-github commented 4 years ago

@tobespc Tried to make codebeat happy, I assume its a prereq for merging?

sam-github commented 4 years ago

@tobespc @rwalle61 diff is uncovered because I moved original code (that was uncovered). Since the coverage is unrelated to this PR, can we merge this? Its blocking https://github.com/appsody/stacks/issues/447

sam-github commented 4 years ago

codebeat hates onIoConnection(), but its existing code, and I'm not sure how to make it happy.

@tobespc PTAL

mattcolegate commented 4 years ago

I'll have a look as Toby's not in today

sam-github commented 4 years ago

@mattcolegate look merge and publishable now? checks have passed.

sam-github commented 4 years ago

Thanks @tobespc