-
Notifications
You must be signed in to change notification settings - Fork 21
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
Bugfix/52 app details v2 support and metrics fix #58
Bugfix/52 app details v2 support and metrics fix #58
Conversation
The App Details page was not working because it was trying to use functionality from the old V1 API (for example fn routes). This commit changes the App Details page to use V2's fns API endpoint and removes the routes logic. Users can now add/edit/remove/run functions for a particular app. This contributes to: fnproject#52, fnproject/fn#1347, and fnproject/fn#1258.
The fn server's metrics API has changed meaning that UI's charts were no longer working. Update the UI to support the new format. This commit: * Fixes the charts on the app listing page. * Removes the deprecated fn route logic from the graphs. * Removes the broken charts from the app details page - this is because the stats that were being used are no longer broken down per function. * Removes the Failed functions graph - this metric is no longer being recorded by the server. This contributes to: fnproject#52 and fnproject/fn#1258.
The App Details page used to display charts that showed metrics on the functions that are part of the App. However, the fn metrics API changed and no longer included this information so it was removed. In fnproject/fn#1365 some very similar metrics were added to the metrics API so add the chart showing the currently running functions back and update it to use this metric. Furthermore, refactor controllers/stats.js to make it easier to parse additional stats in the future. This contributes to: fnproject#52 and fnproject/fn#1258.
Reduce the amount of duplication by merging the graph components into one component.
I'm not convinced the App Details graphs are going to scale when there are lots of functions. By using the function name rather than breaking it down into its own individual images it should improve performance a little bit as there is less to plot (although maybe not enough). Furthermore, it seems unlikely old images will be used so it's wasted information. Lastly, it might be possible for functions to share images which would make it difficult to differentiate between these.
The code that adds the legends to the charts was removed in b83b3eb because the charts were no longer broken down by app. Now we have charts that have per function series data then add the legend code from this commit back.
The Fn Server is records information about which functions are starting, waiting, idling and paused. Chart this information on the App Details page along with the busy chart.
Hi. As you may know, metrics data is not based on the whole Fn cluster data. With distributed Fn, metrics would be different on each node of a cluster. We still sorting out metrics user story for Fn users, until that, metrics view is a lot confusing for those of us who run Fn beyond |
@denismakogon right.. lets restore to where they were then figure out the metrics story afterwards. @vzDevelopment thanks for this, great work. I'm seeing stats on the home screen but nothing on the apps page. I see the outgoing request for /stats returning a 200, but no graphs or numbers changing on the screen. |
@carimura, humm that's interesting. I'm currently on my phone but will take a look when I am back in the office tomorrow and see if I can replicate it. Out of curiosity, what is the content of the fnserver's Prometheus data I.e. If you have stopped the fn server please run the function again before providing the output above. IIRC the Prometheus data doesn't contain metrics for functions until they have been run at least once (from the server start time). Also, how long do the functions you're trying it with take to run? If you add a |
here's what /metrics returns: https://gist.github.com/carimura/6ff0c69c06b975b80d4dc00b10f6c4b3 ya I have a sleep in there of 10 seconds but I'm not getting anything on the app specific page. The stats work great on the home page. |
@carimura ah are you using a custom docker image for the fn server with changes to the views e.g. these lines? If you run I didn't realise that the tags were configurable per build so I'll need to add support for this. The reason the graphs aren't working for you is because the app and fn IDs aren't being returned with
Whereas you get:
|
For reference I'm using:
|
Now this is where my unfamiliarity with fn is going to let me down and am expecting you'll know more than me. In order to get the The small functions I use normally start up in fractions of a second so it's very hard to ever see anything in the I've never been able to get anything in Idling and am not sure when anything ever goes into that state. In order to test these graphs I mocked data from the fn server's metrics API with non-zero values to check the parsing and displaying is correct. Therefore any issues with these graphs should be due to the fn server rather than the UI. I could do with adding unit tests to my parsers to ensure this but haven't looked at how to write unit tests in Node yet. If there are any graphs you don't think are worth having, let me know and I can take them out. I've just included them because the data was there in the server's API. |
In fnproject/fn#1438 Reed made the tags shown in Fn's metrics API configurable. This means in some custom builds there may not be an appID, fnID, or imageName attribute associated with the fn_container stats. This issue was brought to light in fnproject#58. Update the code so that when the Fn server doesn't provide the required information the interface handles it gracefully, showing a message with possible reasons as to why the stats may not be showing.
Sometimes the Fn Server doesn't return data which a graph uses for example 'fn_container_paused' isn't shown until a function enters the state. This means the Paused graph shows "NaN" and doesn't plot a line. Update the graphs so that when there is no metric, it defaults to 0 rather than undefined.
I have pushed an update to address the configurable tags (#58 (comment)). The changes are as follows: Add a message when the metric doesn't existIf the AppID doesn't exist in the returned API data then it'll show a message saying:
To test this, restart the fn server and browse to the app page. When you run a function in the app the message will go away and the graphs will be displayed. Group unknown functionsIf the fn server isn't configured to return the Funnily enough when I went to grab a screenshot I managed to capture a function entering the To test this, run an fn server with I have also tested all permutations of when a tag does/doesn't exist e.g. when no tags exist, when only a few exist, etc. Fix a bug where it shows NaNThe |
This is awesome. Think we should just get it merged. I've looked through and it looks good and it's certainly a lot better than the state it was in. :) Once merged, there are a few things we can address like a little continuity in which stats we show from "global" view to "app" view. After that, there are a bunch of improvements we can make together. it's been too long since we've had a working UI, thanks again @vzDevelopment. |
This fixes the App Details page and the graphs/charts for #52 and fnproject/fn#1258. See #52 (comment) and #52 (comment) for details.
It also contributes to fnproject/fn#1347.
To test the app details page:
To test the graphs:
Something to note is I'm not convinced the graphs on the app details page are going to scale e.g. when there are lots of apps/functions it has to plot. I thought we can try it and if we find they don't we can just remove them. This is also the reason I'm not using these more fine-grained stats on the home page too - although it'd be nice to show a break down per app, I'm worried it's going to be a resource hog.
We can probably close fnproject/fn#1258 after merging this PR and I'll continue the rest of the work in #52.