I’ve had some previous discussions about it here before, and much thanks to @LostKobrakai and @doomspork for their help, but I’m on the verge of opening the contents of this PR as a “real” one upstream, and a) thought some pre-review from the community might save the maintainers time, and b) there’s still one subtle bug, I think in the JS code, that I can’t figure out with the charts not rendering correctly until a “live”/non-historical datapoint gets sent to the client.
So any reviews or thoughts, in the PR or here, would be appreciated! This PR is just against my fork’s master, if/when we get bug and any other feedback from community here addressed, I’ll open it for real upstream. Thanks all!
I believe some of the points I have made on the initial PR still have to be addressed:
Storing the whole measurement + metadata is going to be expensive, therefore the historical API should not be requested to return said information. They should return the label and values only.
historical_data can be a MFA that applies to all events, instead of each event individually.
Oh, man, was trying to do this to avoid bothering you José, but thank you for looking anyway!
re: 1, the metadata storage is optional, and it only seems useful for certain chart types at the moment, so I don’t store it for anything but the phoenix ones… it defaults to empty map when not present. I was thinking about pruning the metadata to only whats useful in the storage side, and ideally exposing some kind of API in dashboard to allow hook to know what it needs to keep from metadata for charts, but thanks for reminder to look further into that.
re 2, yeah, I could work on that as an optimization, though it adds complexity, and what we’re seeing on elixir-companies so far is <edited>I said neglibible but I forgot we just deployed the only part that saves metadata, and that is showing some increased memory impact… not horrendous, but, probably worth working on…
Oh, and I figured out you meant module-function-arguments by MFA, and makes more sense now… not sure I see yet how to rework that way, but I’ll give it a shot…
</edited> I think I get the gist, though if you have any specific resources or reading around that term or your idea here happy to do some reading etc.
the previous version was only returning label and values, but I thought that created too much knowledge outside of dashboard about how it would consume or format them, but I guess given what I exposed as public in this I can use just as easily outside dashboard to pre-convert the telemetry data into labels and values, and then pass that along on callback… I’ll work on that next, thanks!
Apparently I didn’t fully digest/understand some of that, and still am not sure I do, so if anyone else wants to elaborate/expand on Jose´s points here or there, I will be receptive. Hopefully won’t take any more of his time here or there, intentionally or otherwise, until this is closer to meeting his goals, I think I have a handle on some of them, namely 1) changing from the API proposed by LostKobrakai to a simpler one of only providing a single MFA tuple to always call for all historical data on all metrics, and leave it up to that method to dispatch or return empty list based on metric, and 2) do the transformation into label and value outside of dashboard inside the hook before saving, to minimize memory impact. If anyone sees anything in either place this misses as feedback, or has thoughts on best way to accomplish this cleanly, I’m all ears/open to collaboration, but I think I have most of a path forward at least functionally to iterate towards that.
Above is now merged into the main branch/PR and working well, definitely still planning some more tweaks and cleanup before final PR, but additional feedback definitely welcome in updated state… it’s also a definite improvement since all you need now is to pass the metrics you want history for, so could easily extract into a small library that just takes some config and handles defining your live_dashboard route for you… maybe I’ll whip that up next to make easier for anyone to play with in their project before this gets info upstream/master
For anyone else who’d like to try this in their project, eventually when above PR is merged I will publish this on Hex, but for now this is usable as a git dependency in your mix.exs, feedback on docs or code welcome:
