Skip to content
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

chore: update docs for new alerts and reporting feature #13104

Merged
merged 18 commits into from
Feb 26, 2021
Merged

chore: update docs for new alerts and reporting feature #13104

merged 18 commits into from
Feb 26, 2021

Conversation

leocape
Copy link
Contributor

@leocape leocape commented Feb 13, 2021

SUMMARY

Added docs for setting up and configuring the new alerts and reports in V.1.0

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Using the templates outlined in the doc, run these steps to get the new alerts and reporting feature working. After these steps are completed, you should now be able to create and test alerts and reports

  1. Create a new directory and create the Dockerfile
  2. Build the extended image using the Dockerfile
  3. Create the docker-compose.yaml file in the same directory
  4. Create a new sub directory called config
  5. Create the superset_config.py file in the config sub directory
  6. Run the image using docker-compose up in the same directory as the docker-compose.py file

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

fix bullets
fix redis depends on name in docker-compose template
@amitmiran137 amitmiran137 changed the title update docs for new alerts and reporting feature chore: update docs for new alerts and reporting feature Feb 13, 2021
@Yann-J
Copy link
Contributor

Yann-J commented Feb 14, 2021

FYI, I have just sent PR #13116 which would enable this on Kubernetes by (optionally) running Celery beat as a separate singleton pod.

The rest of the configuration can be injected using the existing chart mechanism:

  • Install webdrivers by overriding the worker's command (although doing this at startup time isn't great... the best would be to have them preinstalled in the image...)
  • Pass all the necessary superset_config.py overrides from values.yaml

step added: Upgrade the db
step added: init
step added: setup admin
fix redis superset naming
add . to docker build command
change postgres to internal so it persists on docker-compose down
@@ -6,8 +6,309 @@ index: 10
version: 1
---

## Scheduling and Emailing Reports
## Alerts and Reports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really comfy about putting the Alerts & Reports doc in a page named email-reports, as this doc concern alerts too, and not only emails but also Slack 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd definitely agree, was really just for organisation so that in the menu there isn't 'Alerts and reporting' and then below it 'Scheduling and Emailing reports' which would be confusing imo, but if @srinify can help we could put it into its own section 'Alerts and Reporting', and then have the legacy reporting stuff remain at the bottom of this file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, renaming and keep the old approach at the end seems the best!

@nytai
Copy link
Member

nytai commented Feb 15, 2021

@leocape this is an excellent community effort in documenting a highly requested and very confusing feature set. I'm wondering how much of the config changes can be included in the default superset files (superset/config.py, Dockerfile, docker-compose.yml), so only minimal changes are required to enable this feature for testing. Documentation can then focus more on how to deploy this in prod-like environments (ie, not using docker-compose). Thoughts @dpgaspar? Are there any blockers (licensing) to adding the browser + webdriver to the Dockerfile?

@nytai
Copy link
Member

nytai commented Feb 16, 2021

I also opened #13143 which includes a new docker-compose file for non-dev use cases. I added a beat container which I realized was missing in the regular docker-compose workflow.

@leocape
Copy link
Contributor Author

leocape commented Feb 16, 2021

@leocape this is an excellent community effort in documenting a highly requested and very confusing feature set. I'm wondering how much of the config changes can be included in the default superset files (superset/config.py, Dockerfile, docker-compose.yml), so only minimal changes are required to enable this feature for testing. Documentation can then focus more on how to deploy this in prod-like environments (ie, not using docker-compose). Thoughts @dpgaspar? Are there any blockers (licensing) to adding the browser + webdriver to the Dockerfile?

Thanks @nytai with all the help from everyone on the Slack threads. Yes would be great to simplify this doc so that it focuses more/only on the feature related config. I could cut out the superfluous docker-compose and dockerfile if you can include the webdriver (and postgres?) into the default dockerfile?

@leocape
Copy link
Contributor Author

leocape commented Feb 16, 2021

#13116

Thanks @Yann-OAF I could link to it from this doc where I've placed the link placeholder?

mv chromedriver /usr/bin && \
apt autoremove -yqq --purge && \
apt clean && \
rm -f google-chrome-stable_current_amd64.deb chromedriver_linux64.zip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, would you have similar working instructions to install geckodriver? My attempts have been unsuccessful...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get it working with geckodriver/firefox, no matter which version I tried, it wouldn't launch correctly when trying to take the screenshot... I suspect there is a missing config somewhere that is needed for it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works with geckodriver too, we can adapt the doc to propose both approaches.

@Yann-J
Copy link
Contributor

Yann-J commented Feb 16, 2021

#13116

Thanks @Yann-OAF I could link to it from this doc where I've placed the link placeholder?

Yes, sounds good. These are probably too much to integrate to this doc, and won't be of interest to everyone (only k8s users) so linking is probably enough! I'm thinking I might start a dedicated doc on running in Kubernetes

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leocape this is an excellent community effort in documenting a highly requested and very confusing feature set. I'm wondering how much of the config changes can be included in the default superset files (superset/config.py, Dockerfile, docker-compose.yml), so only minimal changes are required to enable this feature for testing. Documentation can then focus more on how to deploy this in prod-like environments (ie, not using docker-compose). Thoughts @dpgaspar? Are there any blockers (licensing) to adding the browser + webdriver to the Dockerfile?

I think it does not make sense to release a dockerfile with an webdriver + browser installed, and that's where we need to exclude all non compliant ASF packages/software. So IMO we should keep the docker file lean and clean, and add hooks so that it's easy to add additional packages or software (on this case)

btw, chrome driver license is 3-BSD, so it's "compatible" with ASF.

RUN apt update
RUN wget https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb && \
apt install -y --no-install-recommends ./google-chrome-stable_current_amd64.deb && \
wget https://chromedriver.storage.googleapis.com/88.0.4324.96/chromedriver_linux64.zip && \
Copy link
Member

@dpgaspar dpgaspar Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two versions will eventually differ

@nytai
Copy link
Member

nytai commented Feb 16, 2021

Ok, in that case we should keep the Dockerfile changes. I'm planning to add the beat container in #13143 so that should take care of some the docker-compose changes, though it will need to be updated to reference the new images.

4. Create a new sub directory called `config`
5. Create the `superset_config.py` file in the `config` sub directory
6. Run the image using `docker-compose up` in the same directory as the `docker-compose.py` file
7. In a new terminal window, upgrade the DB by running `docker exec -it superset-1.0.1-extended superset db upgrade`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a better way to run

superset db upgrade
superset init
superset fab create-admin

as part of startup without entering the container?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the superset-init service in the docker-compose workflow should take care of this.

@srinify
Copy link
Contributor

srinify commented Feb 26, 2021

Hey @vnourdin @leocape I'm merging this as-is for now! The clean up process is quite involved, I like some things about the older commit and the latest commit and there's a lot of context to resolve. For now, it'd be nice to just have the info out there.

So I'm merging for now, and I'll do a second pass next week to clean up and organize better!

@srinify srinify merged commit 1697e1e into apache:master Feb 26, 2021
@Kingflyinger
Copy link

Kingflyinger commented Feb 27, 2021

version: '3.6' services: redis-superset: image: redis:6.0.9-buster restart: on-failure
redis-superset should be replaced by redis,as well as superset_config.py.
or change the depends_on .

and after done all, what`s the account and password? I tried admin/admin, wrong

allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* update docs for new alerts and reporting feature

* fix punctuation

* fix punctuation

* fix punctuation

fix bullets
fix redis depends on name in docker-compose template

* fix bullets

* Add extra steps for getting v1.0.1 running

step added: Upgrade the db
step added: init
step added: setup admin
fix redis superset naming
add . to docker build command

* fix worker to use gevent instead of prefork

change postgres to internal so it persists on docker-compose down

* add comments to superset_config.py

* ref: Re-organize a bit and make it easier to read

* ref: rm useless level

* docs: md changes + more details + kubernetes

* docs: Rename to Alerts and Reports

* docs: Link to Kubernetes doc

* docs: details for docker-compose + minor md changes

* docs: fix lists indent

* docs: remove unused config

* docs: Put detailed config and dockerfile before specific stuff

* docs: clean Dockerfiles

Co-authored-by: Valentin NOURDIN <vnourdin.externe@bedrockstreaming.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants