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

Fix for telemetry in sharaeable viz #1551

Merged
merged 4 commits into from
Oct 4, 2023

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Oct 3, 2023

Description

Fixes an issue with telemetry in sharaeble viz uploaded site

Development notes

  • Ingest telemetry script to index.html file on s3

QA notes

  • In your demo-project/.telemetry file, edit the consent to true
  • Hit the deploy api via curl or Postman
  • The telemetry script is ingested into index file and you can see heap api calls in the Network tab

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: ravi-kumar-pilla <[email protected]>
injected_head_content.append("</head>")
html_content = html_content.replace("</head>", "\n".join(injected_head_content))

with tempfile.TemporaryDirectory() as temp_dir:
Copy link
Contributor

Choose a reason for hiding this comment

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

great!

with tempfile.TemporaryDirectory() as temp_dir:
temp_file_path = f"{temp_dir}/index.html"

with fsspec.open(temp_file_path, "w", encoding="utf-8") as temp_index_file:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
with fsspec.open(temp_file_path, "w", encoding="utf-8") as temp_index_file:
with open(temp_file_path, "w", encoding="utf-8") as temp_index_file:

Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this. It works well and tracking gets added as it should!

Have a look at the tests though. They're failing.

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

LGTM.
I was thinking since we are ingesting heap analytics in two places, we could possibly abstract it. Having said that, for that might need a utils.py and we don't have that and that approach makes it bit more complex. Anyhow, according to the rule of three .. this should be fine :)
Once we extend and build deployer class. These functions can be part of the baseDeployer.

@ravi-kumar-pilla ravi-kumar-pilla merged commit ded96ff into feature/shareable-urls Oct 4, 2023
1 check passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the fix/shareable-telemetry branch October 4, 2023 15:48
tynandebold added a commit that referenced this pull request Oct 10, 2023
* nodes working

Signed-off-by: Rashida Kanchwala <[email protected]>

* added pipeline

Signed-off-by: Rashida Kanchwala <[email protected]>

* fix path

Signed-off-by: Rashida Kanchwala <[email protected]>

* fix bug and lint

Signed-off-by: Rashida Kanchwala <[email protected]>

* fix lint

Signed-off-by: Rashida Kanchwala <[email protected]>

* Add deploy button, modal, and form fields

Signed-off-by: Tynan DeBold <[email protected]>

* Fix failing tests

Signed-off-by: Tynan DeBold <[email protected]>

* Fix failing tests

Signed-off-by: Tynan DeBold <[email protected]>

* Update single file to S3 via file input

Signed-off-by: Tynan DeBold <[email protected]>

* Update test case

Signed-off-by: Tynan DeBold <[email protected]>

* Remove package lock changes

Signed-off-by: Tynan DeBold <[email protected]>

* Hide exp tracking and deploy button when not running Viz locally; remove WS/subscription code

Signed-off-by: Tynan DeBold <[email protected]>

* Add the beginnings of a /deploy REST endpoint

Signed-off-by: Tynan DeBold <[email protected]>

* Trying to send credentials to the backend

Signed-off-by: Tynan DeBold <[email protected]>

* update responses

* test

* Update UI to remove unneeded fields

Signed-off-by: Tynan DeBold <[email protected]>

* Fix failing text

Signed-off-by: Tynan DeBold <[email protected]>

* Workable Prototype

* fix rebase

* fixing stuff

* Successfully returning a 200 from /api/deploy endpoint

Signed-off-by: Tynan DeBold <[email protected]>

* Remove aws sdk package

Signed-off-by: Tynan DeBold <[email protected]>

* backend fixes

* remove map files when uploading

* fix issue on html folder

* UI updates for loading, success, error states; resetting modal

Signed-off-by: Tynan DeBold <[email protected]>

* fix lowerbound on fsspec

* refactored to latest

* some refactor

* fix lint - WIP

* refactor work

* fix lint

* remove old code

* further refactor

* add error handling and debugging

* fix based on review

* modify upload static files logic

* refactor upload api with latest fsspec

* fix unit tests_1

* revert os logic to pathlib

* fix static folder issue

* fix format and lint errors

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add unit tests for shareable viz s3deployer

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add pytests for responses module

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add s3fs as dependency

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add temporary no cover for apps

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update lower reqs

* update fsspec

* check kedro latest version as 18.0 in e2e tests

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update fsspec and s3fs requirements to support earliest kedro version

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add timestamp file for deploy

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add pytest for timestamp route

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint and format errors

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix server changes and test e2e scenarios

Signed-off-by: ravi-kumar-pilla <[email protected]>

* try to catch versionConflicterror

* Style updates; sending updated args to the backend

Signed-off-by: Tynan DeBold <[email protected]>

* Re-deploy flow

Signed-off-by: Tynan DeBold <[email protected]>

* add route /api/project-metadata to provide package version info

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove frontend build for backend unit tests

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove s3fs requirement to test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add s3fs without specific version

Signed-off-by: ravi-kumar-pilla <[email protected]>

* adjust requirements and add pytests for project metadata

Signed-off-by: ravi-kumar-pilla <[email protected]>

* test open s3fs requirement

Signed-off-by: ravi-kumar-pilla <[email protected]>

* test open s3fs requirement

Signed-off-by: ravi-kumar-pilla <[email protected]>

* test open s3fs requirement

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Move share viz button to new location; add download icon

Signed-off-by: Tynan DeBold <[email protected]>

* Update tests

Signed-off-by: Tynan DeBold <[email protected]>

* Add shareable timestamp component

Signed-off-by: Tynan DeBold <[email protected]>

* Update timestamp fetch

Signed-off-by: Tynan DeBold <[email protected]>

* add version info and modify route name from /api/timestamp to /api/deploy-viz-metadata

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Rename file; add version to timestamp

Signed-off-by: Tynan DeBold <[email protected]>

* Add back correct isRunningLocally check

Signed-off-by: Tynan DeBold <[email protected]>

* Remove console.log and update endpoint url

Signed-off-by: Tynan DeBold <[email protected]>

* add pytests for updated api

Signed-off-by: ravi-kumar-pilla <[email protected]>

* undo all new requirements

* undo all fsspec changes

* added s3fs as dependency

* fix unit tests

* clean up tests

* lint

* fix lint

* fix test and lint and compatibility response

* add packaging

* packaging reqs

* Consume compatibility endpoint; use Dropdown for bucket regions

Signed-off-by: Tynan DeBold <[email protected]>

* Update failing test

Signed-off-by: Tynan DeBold <[email protected]>

* Run format-fix

Signed-off-by: Tynan DeBold <[email protected]>

* Remove unused import

Signed-off-by: Tynan DeBold <[email protected]>

* fix lint and router link

* Update API endpoint

Signed-off-by: Tynan DeBold <[email protected]>

* Add try/catch; remove console.log

Signed-off-by: Tynan DeBold <[email protected]>

* Update e2e test

Signed-off-by: Tynan DeBold <[email protected]>

* Update e2e test, again

Signed-off-by: Tynan DeBold <[email protected]>

* Drodown component update

Signed-off-by: Tynan DeBold <[email protected]>

* fix api endpoint

* fixes based on reviews

* fixes based on reviews

* changes based on reviews

* fix lint

* shareable URL modal UI Fixes (#1537)

* shareable url modal UI fix

* Build error fix

* shareable URL modal form validation fix

* fixes based on review

* updated cli help definition

* update filpath to directory

* update filpath to directory

* Add the first of the Cypress tests

Signed-off-by: Tynan DeBold <[email protected]>

* add s3 protocol in the backend

* leftover from merge fix

* Changing language from deploy to publish

Signed-off-by: Tynan DeBold <[email protected]>

* Update wording again; return a better error message to the FE; handle the error; move s3 bucket regions to config file

Signed-off-by: Tynan DeBold <[email protected]>

* Add go back button to error modal stategs

Signed-off-by: Tynan DeBold <[email protected]>

* Update tests; use fstrings; remove unused CSS

Signed-off-by: Tynan DeBold <[email protected]>

* E2E cypress test for Shareable URL modal (#1543)

* E2E cypress test added for shareable

* Renaming from "Deploy" to "Publish"

* Code review changes added

* update pip fsspec

* Update UI with docs links; update feature hits

Signed-off-by: Tynan DeBold <[email protected]>

* Update feature hints for when Viz is deployed

Signed-off-by: Tynan DeBold <[email protected]>

* Update release notes

Signed-off-by: Tynan DeBold <[email protected]>

* Update release notes again

Signed-off-by: Tynan DeBold <[email protected]>

* Add tracking to create_api_app_from_file

Signed-off-by: Tynan DeBold <[email protected]>

* fix e2e tests

* Revert changes to create_api_app_from_file function

Signed-off-by: Tynan DeBold <[email protected]>

* Fix for telemetry in sharaeable viz (#1551)

* fix telemetry for sharaeable viz

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint issue

Signed-off-by: ravi-kumar-pilla <[email protected]>

---------

Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: Tynan DeBold <[email protected]>
Co-authored-by: Tynan DeBold <[email protected]>

* Design QA changes

Signed-off-by: Tynan DeBold <[email protected]>

* Update release file

Signed-off-by: Tynan DeBold <[email protected]>

* PR review fixes

Signed-off-by: Tynan DeBold <[email protected]>

* Add back graphql subscription (removal moved to another PR)

Signed-off-by: Tynan DeBold <[email protected]>

* Update cypress test

Signed-off-by: Tynan DeBold <[email protected]>

---------

Signed-off-by: Rashida Kanchwala <[email protected]>
Signed-off-by: Tynan DeBold <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Co-authored-by: Rashida Kanchwala <[email protected]>
Co-authored-by: Rashida Kanchwala <[email protected]>
Co-authored-by: rashidakanchwala <[email protected]>
Co-authored-by: ravi-kumar-pilla <[email protected]>
Co-authored-by: Jitendra Gundaniya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants