search

Smashing / smashing

The exceptionally handsome dashboard framework in Ruby and Coffeescript.
https://smashing.github.io/
MIT License
3.22k stars 324 forks source link

Send only data that is needed on the dashboard #173

Closed toy closed 3 years ago

toy commented 4 years ago

Get list of data ids after all widgets are created and send it as parameter to event stream endpoint so it sends only requested data. This is especially useful when one app has multiple dashboards which consume different data.

kinow commented 3 years ago

Hi @toy sorry for the delay to look into this one. I am planning a dev cycle for smashing, and to include this PR in the next release. Just need to test/review it.

toy commented 3 years ago

Merged so there is no conflict in the way

kinow commented 3 years ago

Merged so there is no conflict in the way

Thanks! Although GH UI's still showing "This branch cannot be rebased due to conflicts"?

toy commented 3 years ago

Resolving in a merge commit doesn't necessarily help git to resolve same conflicts when rebasing

toy commented 3 years ago

Found out that special handling of Hash setting values by sinatra was the culprit: https://github.com/sinatra/sinatra/blob/d39b135c67b134bd3c8fea636f244d77422e18bc/lib/sinatra/base.rb#L1283 I'm not sure what would be the right way to handle dashboard events, to allow all of them to pass through as I did, or to add the id of the dashboard to ids to consume.

toy commented 3 years ago

LGTM

kinow commented 3 years ago

LGTM

Merging in a few minutes with the new commits. Will leave a message in our Gitter/Matrix chat room so others are aware of this change, and I'll ask them to report in case they have any issue due to this new feature (don't think that will happen :smiley: ).

Thanks heaps @toy !

kinow commented 3 years ago

Rebased, added 2 commits locally, merged branch with master, and pushed all commits there too :+1: merged! Release is out is 15 minutes, just going through changelog & checklist.

mikewalkerblackpatch commented 3 years ago

@kinow @toy Sorry not sure if this is the correct place to raise an issue for this. Since this change some of my (very large) dashboards are not returning all the data I think the Get/events?ids request is too big? I hadn't see this issue in v 1.3.0 so came looking here to see whats changes.

I can see the request output when I am running locally, (included below) it's less than half of all the ids I need for that dashboard. Can you help?

127.0.0.1 - - [02/Mar/2021:13:11:22 +0000] "GET /events?ids=applivery%2Capplivery.sit%2Capplivery.uat%2Capplivery.preprod%2Capplivery.live%2Cclient-api%2Cclient-api.sit%2Cclient-api.uat%2Cclient-api.preprod%2Cclient-api.live%2Ccollection-streams-processor%2Ccollection-streams-processor.sit%2Ccollection-streams-processor.uat%2Ccollection-streams-processor.preprod%2Ccollection-streams-processor.live%2Ce2e-parcel-events-connector%2Ce2e-parcel-events-connector.sit%2Ce2e-parcel-events-connector.uat%2Ce2e-parcel-events-connector.preprod%2Ce2e-parcel-events-connector.live%2Ce2e-tour-reporting-sink-connector%2Ce2e-tour-reporting-sink-connector.sit%2Ce2e-tour-reporting-sink-connector.uat%2Ce2e-tour-reporting-sink-connector.preprod%2Ce2e-tour-reporting-sink-connector.live%2Cemail-sender%2Cemail-sender.sit%2Cemail-sender.uat%2Cemail-sender.preprod%2Cemail-sender.live%2Chht-auth-service%2Chht-auth-service.sit%2Chht-auth-service.uat%2Chht-auth-service.preprod%2Chht-auth-service.live%2Chht-config-api%2Chht-config-api.sit%2Chht-config-api.uat%2Chht-config-api.preprod%2Chht-config-api.live%2Chht-event-feed%2Chht-event-feed.sit%2Chht-event-feed.uat%2Chht-event-feed.preprod%2Chht-event-feed.live%2Chht-parcel-api%2Chht-parcel-api.sit%2Chht-parcel-api.uat%2Chht-parcel-api.preprod%2Chht-parcel-api.live%2Chht-routing-api%2Chht-routing-api.sit%2Chht-routing-api.uat%2Chht-routing-api.preprod%2Chht-routing-api.live%2Chht-tour-api%2Chht-tour-api.sit%2Chht-tour-api.uat%2Chht-tour-api.preprod%2Chht-tour-api.live%2Ckafka-delayed-topics%2Ckafka-delayed-topics.sit%2Ckafka-delayed-topics.uat%2Ckafka-delayed-topics.preprod%2Ckafka-delayed-topics.live%2Cmars-et-services%2Cmars-et-services.sit%2Cmars-et-services.uat%2Cmars-et-services.preprod%2Cmars-et-services.live%2Cmars-opunits-connector%2Cmars-opunits-connector.sit%2Cmars-opunits-connector.uat%2Cmars-opunits-connector.preprod%2Cmars-opunits-connector.live%2Cmars-parcel-feed%2Cmars-parcel-feed.sit%2Cmars-parcel-feed.uat%2Cmars-parcel-feed.preprod%2Cmars-parcel-feed.live%2Cmars-parcel-feed-jdbc-connectors%2Cmars-parcel-feed-jdbc-connectors.sit%2Cmars-parcel-feed-jdbc-connectors.uat%2Cmars-parcel-feed-jdbc-connectors.preprod%2Cmars-parcel-feed-jdbc-connectors.live%2Cmars-parcel-target-hist-connector%2Cmars-parcel-target-hist-connector.sit%2Cmars-parcel-target-hist-connector.uat%2Cmars-parcel-target-hist-connector.preprod%2Cmars-parcel-target-hist-connector.live%2Cparcel-date-processor%2Cparcel-date-processor.sit%2Cparcel-date-processor.uat%2Cparcel-date-processor.preprod%2Cparcel-date-processor.live%2Croute-analysis-connector%2Croute-analysis-connector.sit%2Croute-analysis-connector.uat%2Croute-analysis-connector.preprod%2Croute-analysis-connector.live%2Croute-analysis-tour-data-connector%2Croute-analysis-tour-data-connector.sit%2Croute-analysis-tour-data-connector.uat%2Croute-analysis-tour-data-connector.preprod HTTP/1.1" 200 - 90.1307

kinow commented 3 years ago

@mikewalkerblackpatch hmm, should have considered that possibility. I think we might have to revert the change, cut a release, and open an issue to investigate having the ids=a,b,c as an optional feature. Any thoughts @toy ?

toy commented 3 years ago

@mikewalkerblackpatch I think normally it is better to create an issue (there is one 🙂 ), but upto @kinow

About the issue - first I'm impressed, there are 104 ids in the the url and I didn't consider so many widgets. I'm wondering what exactly happens for those dashboards, as they shouldn't receive any data as too long urls should cause status 414 from the server. And too long url is not defined by spec, while from a bit of searching it seems that the safe bet is 2000 chars, but for example thin used by default by smashing has a limit of 1024 * 10 for query string, so maybe a solution is to check and tweak configs for servers/proxy-servers on the way to smashing.

@kinow It is still a good idea to make it possible to receive all data, probably some option like Dashing.filterEvents = false which causes request to not have ids parameter which is understood by backend as a signal to send all events. For now there is only one know issue, but up to you to revert the change.

mikewalkerblackpatch commented 3 years ago

@toy we actually have 168 data-id's on this particular dashboard. We amended the Jenkins Build widget to contain 4 data points (one for each environment) here is just one as an example. image image

With v 1.3.2 the dashboard was still rendering ok, but there was no data for all widgets past the 104. I'm going to try renaming the id's to be much shorter so maybe that would help us if there is a 2000 character limit. In the meantime, we've managed to convince the westwater/smashing docker container to pull down v 1.3.0 rather than the latest when we start it up.

@kinow Now I have worked out how, I am happy to raise this as an issue if you like.

kinow commented 3 years ago

Let's create a new issue. My plan is to work on a fix as @toy suggested that allows to send all events data.

Let's discuss it there and figure out if we use a new setting, if that's enabled or not by default, or any other approach. Thanks!

mikewalkerblackpatch commented 3 years ago

I have opened issue https://github.com/Smashing/smashing/issues/181 Many thanks for your help.