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

Added monitoring namespace based on pid, tid and hostname #1222

Merged
merged 9 commits into from
Feb 15, 2023
Merged

Added monitoring namespace based on pid, tid and hostname #1222

merged 9 commits into from
Feb 15, 2023

Conversation

Raalsky
Copy link
Contributor

@Raalsky Raalsky commented Feb 14, 2023 •
edited
Loading

Before submitting checklist

  • Did you update the CHANGELOG? (not for test updates, internal changes/refactors or CI/CD setup)
  • Did you asked the docs owner to review all the updated user-facing interfaces?

@Raalsky Raalsky changed the title Draft: Added monitoring namespace based on pid, tid and hostname Added monitoring namespace based on pid, tid and hostname Feb 15, 2023
@Raalsky Raalsky force-pushed the rj/monitoring-namespace branch from efddd11 to 7e28316 Compare February 15, 2023 08:57
@codecov
Copy link

codecov bot commented Feb 15, 2023 •
edited
Loading

Codecov Report

Base: 71.70% // Head: 71.36% // Decreases project coverage by -0.34% ⚠️

Coverage data is based on head (f4589b3) compared to base (caa1ef6).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1222      +/-   ##
==========================================
- Coverage   71.70%   71.36%   -0.34%     
==========================================
  Files         284      285       +1     
  Lines       13639    13656      +17     
==========================================
- Hits         9780     9746      -34     
- Misses       3859     3910      +51
Flag Coverage Δ
e2e ?
unit-pull-request 71.36% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/neptune/internal/init/run.py 90.56% <100.00%> (+0.70%) ⬆️
...une/internal/streams/std_capture_background_job.py 96.15% <100.00%> (-0.15%) ⬇️
src/neptune/internal/utils/hashing.py 100.00% <100.00%> (ø)
...eptune/common/websockets/reconnecting_websocket.py 27.77% <0.00%> (-50.01%) ⬇️
...tune/common/websockets/websocket_client_adapter.py 34.78% <0.00%> (-50.01%) ⬇️
...neptune/legacy/internal/utils/alpha_integration.py 41.66% <0.00%> (-45.84%) ⬇️
...ternal/threads/hardware_metric_reporting_thread.py 39.13% <0.00%> (-43.48%) ⬇️
...ernal/websockets/reconnecting_websocket_factory.py 57.14% <0.00%> (-42.86%) ⬇️
...tune/legacy/internal/streams/stdstream_uploader.py 46.87% <0.00%> (-40.63%) ⬇️
src/neptune/common/patches/bravado.py 38.88% <0.00%> (-38.89%) ⬇️
... and 155 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Raalsky
Raalsky commented Feb 15, 2023
View reviewed changes
src/neptune/internal/init/run.py Outdated
@@ -141,25 +143,25 @@ def init_run(
Unix style pathname pattern expansion is supported. For example, you can pass `*.py` to upload
all Python files from the current directory.
If None is passed, the Python file from which the run was created will be uploaded.
capture_stdout: Whether to log the stdout of the run. Defaults to True.
TODO: capture_stdout: Whether to log the stdout of the run. Defaults to True.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@normandy7 I need you here 🚀. After this PR we're going to log all monitoring metadata (CPU, GPU, memory usage, standard console output etc.) under monitoring/<some-hash> for instance monitoring/ab8c2f0d. This hash will be generated based on some environment information (network name of user computer, process identifier, thread identifier). I really have no idea how to rephrase it properly. Could you help me?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. I'll propose something.

@Raalsky Raalsky requested a review from normandy7 February 15, 2023 09:29
aniezurawski
aniezurawski requested changes Feb 15, 2023
View reviewed changes
src/neptune/internal/init/run.py Show resolved Hide resolved
src/neptune/internal/utils/hashing.py Outdated
import hashlib


def generate_hash(*descriptors, length: int = 8):
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 a fan of such default value. Probably should be explicitly set to 8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Raalsky Raalsky requested a review from aniezurawski February 15, 2023 12:43
@Raalsky Raalsky merged commit 8df0871 into master Feb 15, 2023
@Raalsky Raalsky deleted the rj/monitoring-namespace branch February 15, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normandy7 normandy7 normandy7 approved these changes

@aniezurawski aniezurawski aniezurawski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Raalsky @aniezurawski @normandy7