-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix/dashboard #227
Fix/dashboard #227
Conversation
…and responses; update test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of my suggested changes are to yaml configuration files.
I'm not sure if they can be applied directly. I think it's better to apply corresponding changes via superset web app and export new config files.
dff/config/superset_dashboard/dashboards/DFF_statistics_dashboard_1.yaml
Outdated
Show resolved
Hide resolved
dff/config/superset_dashboard/charts/Current_topic_slot_bar_chart_4.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Roman Zlobin <[email protected]>
Co-authored-by: Roman Zlobin <[email protected]>
…to Superset documentation
dff/config/superset_dashboard/dashboards/DFF_statistics_dashboard_1.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to not use actions for that.
Because the arguments are required, users are forced to specify them in the command (dff.stats config.yaml -U superset -P -dP
).
I think it is better to make them not required and then process them inside the main
function instead of processing with PasswordAction
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current way is better, because it allows for clearer code; the face that users have to pass -U and -P does not sound like a concern, since that is the way it functions in MySQL, Postgresql and all kinds of other systems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a temporary file instead.
Otherwise it will delete temp.zip
file in the current directory if it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with uuid random name
dff/config/superset_dashboard/datasets/dff_database/dff_node_stats.yaml
Outdated
Show resolved
Hide resolved
…; make a variable for superset metadata db; adjust dockerfile to use configuration overrides
dff/config/superset_dashboard/charts/Flow_visit_ratio_monitor_13.yaml
Outdated
Show resolved
Hide resolved
dff/config/superset_dashboard/datasets/dff_database/dff_stats.yaml
Outdated
Show resolved
Hide resolved
Apply suggestions from @RLKRo Co-authored-by: Roman Zlobin <[email protected]>
Apply suggestions to label_lag/flow_lag Co-authored-by: Roman Zlobin <[email protected]>
…om charts; update docker-compose healthcheck; apply formatting
tests/stats/test_main.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in the light of the bugs I've discovered we should really test the charts.
Bugs such as request_id
s being sorted as strings are difficult to catch by simply looking at the charts which makes them all the more dangerous as they still present wildly incorrect data.
I think we should prioritize finding a way to test the data in these charts.
@@ -66,13 +87,21 @@ services: | |||
- '9000:9000' | |||
volumes: | |||
- ch-data:/var/lib/clickhouse/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird that only clickhouse has its own volume.
Why not give a volume to all db containers?
Restarting all containers still resets all the metadata stored in dashboard-metadata
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that adding volumes to all containers lies outside the scope of this PR, so for now I will only add volumes for clickhouse and dashboard-metadata
@@ -2,6 +2,7 @@ | |||
export SERVER_THREADS_AMOUNT=8 | |||
set -m | |||
nohup /bin/bash /usr/bin/run-server.sh & | |||
sleep 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of a simple sleep
we could use something like what we have in make wait_db
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot figure out the exact event that we need to be waiting for; it would be feasible, if we knew what it was
@@ -50,10 +50,31 @@ services: | |||
context: ./dff/utils/docker | |||
dockerfile: dockerfile_stats | |||
image: ghcr.io/deeppavlov/superset_df_dashboard:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should build a local image in tests instead of using a remote image.
(as mentioned here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be addressed by passing the --build
flag to the docker-compose up
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply suggestions by @RLKRo Co-authored-by: Roman Zlobin <[email protected]>
…k expression; create volume for dashboard metadata
# Release notes ## dff.stats - Fix tests for statistics module (#272) - Dashboard config now persists after restart (#227) - Fix several bugs with SQL statements for datasets (#227) - Update guide with information about custom charts (#227) - Add text descriptions for charts in dashboard (#227) - Various chart fixes (#227) ## Other - Update PR template (#258) - Small doc fixesRelease v0.6.2
Description
Create data provider, add new charts to dashboard; rework existing sections
Checklist