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.

Sign up for GitHub

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

Jump to bottom

Loki for voxelytics logs #6770

Merged
merged 21 commits into from
Jan 25, 2023
Merged

Loki for voxelytics logs #6770

merged 21 commits into from
Jan 25, 2023

Conversation

normanrz
Copy link
Member

@normanrz normanrz commented Jan 20, 2023 •
edited
Loading

  • Replaces elasticsearch with loki for vx log storage
  • Changes the log frontend to only show the latest 1000 log entries

Screenshot 2023-01-20 at 20 09 03

URL of deployed dev instance (used for testing):

Steps to test:

(Please delete unneeded items, merge only when none are left open)

@normanrz normanrz self-assigned this Jan 20, 2023
@normanrz
Copy link
Member Author

Not sure whether to show the latest log messages first or last 🤔

fm3
fm3 reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend mostly LGTM, I added some small comments :)

app/models/voxelytics/LokiClient.scala Show resolved Hide resolved
app/models/voxelytics/LokiClient.scala Outdated Show resolved Hide resolved
app/models/voxelytics/LokiClient.scala Outdated Show resolved Hide resolved
fm3
fm3 reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Backend LGTM :) Leaving final approval for frontend review

daniel-wer
daniel-wer reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

This is a nice improvement! 👍 Maybe we can test this with a run that logs a little bit more?

conf/application.conf Show resolved Hide resolved
frontend/javascripts/admin/voxelytics/log_tab.tsx Show resolved Hide resolved
frontend/javascripts/admin/voxelytics/log_tab.tsx Show resolved Hide resolved
frontend/javascripts/admin/voxelytics/log_tab.tsx Show resolved Hide resolved
frontend/javascripts/admin/voxelytics/task_list_view.tsx Show resolved Hide resolved
frontend/javascripts/admin/voxelytics/utils.ts Outdated Show resolved Hide resolved
@normanrz
Copy link
Member Author

Maybe we can test this with a run that logs a little bit more?

I'll let the test suite run tonight.

daniel-wer
daniel-wer approved these changes Jan 23, 2023
View reviewed changes
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

LGTM if the tests run through successfully 👍

@normanrz normanrz requested a review from fm3 January 24, 2023 12:22
@normanrz
Copy link
Member Author

@fm3 Would you mind having another look at my latest commits? I moved the batched-fetching from the frontend to the backend.

fm3
fm3 reviewed Jan 24, 2023
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

LGTM :)
Do you think it is likely that we will want to change the constants POLLING_INTERVAL, LOG_TIME_BATCH_INTERVAL, LOG_ENTRY_BATCH_SIZE again soon? If so, maybe they should be extracted to the config? But if you think this unlikely, feel free to leave as is

@normanrz
Copy link
Member Author

@daniel-wer here is the dev instance with a longer run: https://lokilogs.webknossos.xyz/workflows/e8185485dc?runId=63cfcd920100008b00668fc9

@daniel-wer
Copy link
Member

@daniel-wer here is the dev instance with a longer run: https://lokilogs.webknossos.xyz/workflows/e8185485dc?runId=63cfcd920100008b00668fc9

Thanks, looking good, but I noticed two things:

  • Sometimes text is overlaid on top of each other (seems to only happen for lines that are a tiny bit too long for a single line)
    overlaid_text_bug
  • The last couple of lines of the train model log (and other tasks), actually belong to the next task 
    • 2023-01-24T12:26:07.587Z NOTICE   voxelytics 

🚀🚀🚀 Executing evaluate_segmentation@predict_batch
2023-01-24T12:26:07.588Z NOTICE   voxelytics Metadata for the current workflow execution can be found at 
runs/segem_test_until_predict.2023-01-24_13-22-19

@normanrz
Copy link
Member Author

  • Sometimes text is overlaid on top of each other (seems to only happen for lines that are a tiny bit too long for a single line)
    overlaid_text_bug

That is probably a problem because of the scrollbar, which changes the width of the container. Hard for me to debug on Mac.

  • The last couple of lines of the train model log (and other tasks), actually belong to the next task
  2023-01-24T12:26:07.587Z NOTICE   voxelytics 
  
  🚀🚀🚀 Executing evaluate_segmentation@predict_batch
  2023-01-24T12:26:07.588Z NOTICE   voxelytics Metadata for the current workflow execution can be found at 
  runs/segem_test_until_predict.2023-01-24_13-22-19

That is actually a vx problem. Not sure how to fix it.

@normanrz
Copy link
Member Author

Since we only show 1000 lines max, I decided to get rid of virtualized log list. This should solve the overlapping line problem. @daniel-wer would you mind having another look? https://lokilogs.webknossos.xyz/workflows/e8185485dc?runId=63cfcd920100008b00668fc9

daniel-wer
daniel-wer approved these changes Jan 24, 2023
View reviewed changes
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice, LGTM 🎉

@normanrz normanrz merged commit f85669e into master Jan 25, 2023
@normanrz normanrz deleted the loki-logs branch January 25, 2023 08:05
hotzenklotz added a commit that referenced this pull request Jan 25, 2023
@hotzenklotz
Merge branch 'master' of github.com:scalableminds/webknossos into mor…
ed7565b 
…e-plans

* 'master' of github.com:scalableminds/webknossos:
  correct font in vx dag
  Catch zero-sized buckets in backend, fix http nio timeout handling (#6782)
  Remove debug logging in editable mapping logic (#6783)
  Loki for voxelytics logs (#6770)
bulldozer-boy bot pushed a commit that referenced this pull request Feb 1, 2023
@hotzenklotz
Enforce Pricing for Team Sharing (#6776)
b4eaeed 
* add new blur component around certain paid features
* updated changelog
* Update frontend/javascripts/components/pricing_enforcers.tsx

Co-authored-by: Philipp Otto <[email protected]>
* Merge branch 'master' into more-plans
* Merge branch 'master' of github.com:scalableminds/webknossos into more-plans
* 'master' of github.com:scalableminds/webknossos:
  correct font in vx dag
  Catch zero-sized buckets in backend, fix http nio timeout handling (#6782)
  Remove debug logging in editable mapping logic (#6783)
  Loki for voxelytics logs (#6770)
* re-styled pricing alert
* Merge branch 'master' of github.com:scalableminds/webknossos into more-plans
* 'master' of github.com:scalableminds/webknossos:
  Update linter to Rome v11.0.0 (#6785)
  Fix publicly shared annotations (#6784)
* add isFeatureAllowedByPricingPlan helper function
* enforce pricing for creating precomputed meshes
* use isFeatureAllowedByPricingPlan for SecuredRoute
* disable proof reading tool
* prevent proof reading tool from being unsed through keyboard shortcut
* Merge branch 'master' into more-plans
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fm3 fm3 fm3 left review comments

@daniel-wer daniel-wer daniel-wer approved these changes

Assignees

@normanrz normanrz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@normanrz @daniel-wer @fm3