Add custom metrics and metric history fork to LiveDashboard #613
Conversation
Also rename metric and simplify history to include metadata poller was based on previous custom count logic, doesn’t make sense for shadowing something that works as is without poller.
| @@ -56,6 +56,9 @@ config :companies, CompaniesWeb.Endpoint, | |||
| ] | |||
| ] | |||
|
|
|||
| # Do not run appsignal in local dev | |||
| config :appsignal, :config, active: false | |||
There was a problem hiding this comment.
I know this is technically out of scope, but without this I accidentally got some errors submitted to my work AppSignal account, because I guess I set the env var for APPSIGNAL_PUSH_API_KEY at some point and it was still set... very confusing seeing errors from an unrelated project show up in work project 🙃
| defp pruned_metadata(metadata) do | ||
| # for now keep it all, reminder to either keep or drop selected fields based on dashboard usage to conserve memory, | ||
| # ideally via some published source of truth hook from dahsboard module to ensure correctness | ||
| metadata |
There was a problem hiding this comment.
José expressed concern about memory, and I think this will be the main way we address that concern in practice, though this method will likely need to consider the metric as well as the metadata to decide what to save vs discard... but I think for now we can either leave this as is, or kill, and look at memory usage without discarding anything before deciding if it's a practical issue for us, though we may want to tackle even if it's not a big issue here so that visibility is in place in LiveDashboard to support hooks doing this pruning intelligently.
There was a problem hiding this comment.
On newer versions of my fork, I'm actually just leaving out the metadata entirely and most of the metrics seem to work exactly the same, so possibly we can do the same here, though I'm curious to see the difference in memory impact both ways/it's entirely possible updates to LiveDashboard may start making use of this metadata in new releases that it's not using now.
|
I'm curious if it would make more sense to leverage something existing for the lower-level task of aggregating the metrics so that they're available for querying by LiveDashboard (to fill in the historical data from before the live streaming part takes over). For example, this seems like it does exactly what you'd want to do, but in a more full-featured way: https://github.com/beam-telemetry/telemetry_metrics_prometheus_core I think it's something that you'd want to make configurable if it were to become a built-in part of LiveDashboard, for example, because it's going to store the aggregate data in ETS, but from a brief skim, it looks pretty similar to what you're proposing to do "manually" in a GenServer's state. EDIT: Sorry if I should have posted this to the PR on your actual fork of LiveDashboard. 😅 |
|
Hey Greg, no worries! I’ve looked at that stuff a tiny bit, but thought that was specific to StatsD and getting telemetry immediately to an outside data store so I didn’t look at using it much for this... if you think we can do that more cleanly I’m definitely open to it, but I might need more guidance what you have in mind.. you are talking about the code here, not in live dashboard fork right? The idea is no storage in live dashboard will ever occur, only the hook and formatting of data coming in to the hook on that side, so I think you’re commenting in the right place, but Github was down when I saw this note at my computer and now I’m at the park on mobile, so I’ll look a little more later and see if I understand any better what you have in mind, but either way thanks for the thought! |
|
Looking a tiny bit more @GregMefford I think I maybe partly understand what you're thinking for the aggregation purposes, in that I see that package is sort of agnostic about consumption of the metrics... I'm guessing you're talking mostly about the aggregation going on in Does that clarify/did I get anything wrong there in what you meant? Also, I need to play more, but though I saw the RepoWebHistory charts working for historical data, I also had them fail to load as expected once or twice after I opened the PR while playing around to check memory consumption ... the viewing stats charts are working well and reliably for me, but there may still be something funny with the repo charts, if anyone else can run a little locally and see if a) they see consistently working or not working, and b) if they have any idea what the difference is between when it works and doesn’t, let me know, either way I'll investigate further. We could pull that part out and just merge the view stats and history for now while I keep working on the other stuff for a future PR, up to you guys of course. |
There was a problem hiding this comment.
It seems to work ok. My testing methodology might be dumb, please inform me if so:
for i in `seq 1 20`; do curl http://localhost:4000/en/companies; doneIdk if it's relevant, the only issue is that in dev mode, I saw the graph blank for a few seconds, and then it came back as:
The second time I tried it, it didn't blank. Also, the beam memory stayed cool during the whole process.
LGTM.
|
Thanks @gemantzu ! Was going to add an update to also add VM history, but up to you guys if want that here or a seperate PR... also can probably use style there and stop re-emitting events under a different namespace, not sure why I bothered with that, it's needed for the poller stuff in page_views but not for the repo or VM history |
|
I think you should not add more to this PR. @SophieDeBenedetto can you take a look at this before we merge please? You are far more experienced than me on telemetry :) |
|
I updated my fork with latest upstream master on LiveDashboard, the commit referenced in the mix.lock here 9259f448 is no longer part of the history of that branch because of rebase, but I pushed up that commit in a new branch historical_data_elixir_companies_PR in case anyone is playing/in case mix has trouble pulling it down, don't think there will be any issue, but just in case... |
|
@bglusman given folks limited time these days how do you feel about merging this in now to unblock your other related efforts and we can collectively make improvements to the implementation as we go forward. We'd be glad to have you use this application as a platform to explore LiveDashboard and share those learnings with the community. Happy to discuss other ways we can generate metrics 😁 |
|
Sure Sean, that’s be great! I may try and do a fast follow PR that would be much smaller with a few tweaks but yeah, getting this on prod in some form is the goal.
Whatever works for you guys! Thank you!
…On Wed, Jun 3, 2020 at 9:12 AM Sean Callan ***@***.***> wrote:
@bglusman <https://github.com/bglusman> given folks limited time these
days how do you feel about merging this in now to unblock your other
related efforts and we can collectively make improvements to the
implementation as we go forward.
We'd be glad to have you use this application as a platform to explore
LiveDashboard and share those learnings with the community. Happy to
discuss other ways we can generate metrics 😁
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#613 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAZXZEVIZULFL235TLPRQ3RUZDZZANCNFSM4NH5AFXA>
.
|
|
I'm good to merge this today and go from there. I'll merge now and it should autodeploy. If we run into any hickups I can rollback or ping you. |
|
@doomspork hmm, I guess some issue in the deploy? LMK if I can help, but I don't see it deployed live yet in any case... |
|
@bglusman it looks like there is a compilation issue: == Compilation error in file lib/companies_web/historical_data.ex ==
** (CompileError) lib/companies_web/historical_data.ex:12: unsupported option :reduce given to for
lib/companies_web/historical_data.ex:11: (module)
! Push rejected, failed to compile Elixir app.
! Push failedWe may need to update the Elixir version |
Hey @doomspork / @SophieDeBenedetto This is a working v1 of discussed custom metrics and use of historical data fork of LiveDashboard. Right now it's only saving history for two of the pages of metrics, and one of them is a kind of "duplicate"/fork of the existing Ecto telemetry metrics, so one of them has history one doesn't. Could remove the old/non-saved one or not, but up to you guys! Let me know any thoughts and feel free to review the code on my fork of LiveDashboard as well as part of this, opened a PR into my own master here bglusman/phoenix_live_dashboard#2 !