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

Feature/sudoless metrics #2605

Merged
merged 3 commits into from
Jan 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/streamlit/cli.py
Expand Up @@ -208,7 +208,7 @@ def main_run(target, args=None, **kwargs):
raise click.BadArgumentUsage(
"Streamlit requires raw Python (.py) files, but the provided file has no extension.\nFor more information, please see https://docs.streamlit.io"
)
else:
else:
raise click.BadArgumentUsage(
"Streamlit requires raw Python (.py) files, not %s.\nFor more information, please see https://docs.streamlit.io"
% extension
Expand Down
48 changes: 42 additions & 6 deletions lib/streamlit/metrics_util.py
Expand Up @@ -8,6 +8,9 @@

from streamlit import file_util

_ETC_MACHINE_ID_PATH = "/etc/machine-id"
_DBUS_MACHINE_ID_PATH = "/var/lib/dbus/machine-id"


def _get_machine_id():
"""Get the machine ID
Expand All @@ -25,18 +28,48 @@ def _get_machine_id():
"""
if (
platform.system() == "Linux"
and os.path.isfile("/etc/machine-id") == False
and os.path.isfile("/var/lib/dbus/machine-id") == False
and os.path.isfile(_ETC_MACHINE_ID_PATH) == False
and os.path.isfile(_DBUS_MACHINE_ID_PATH) == False
):
subprocess.run(["sudo", "dbus-uuidgen", "--ensure"])

machine_id = str(uuid.getnode())
if os.path.isfile("/etc/machine-id"):
with open("/etc/machine-id", "r") as f:
if os.path.isfile(_ETC_MACHINE_ID_PATH):
with open(_ETC_MACHINE_ID_PATH, "r") as f:
machine_id = f.read()

elif os.path.isfile("/var/lib/dbus/machine-id"):
with open("/var/lib/dbus/machine-id", "r") as f:
elif os.path.isfile(_DBUS_MACHINE_ID_PATH):
with open(_DBUS_MACHINE_ID_PATH, "r") as f:
machine_id = f.read()

return machine_id


def _get_machine_id_v3() -> str:
"""Get the machine ID

This is a unique identifier for a user for tracking metrics in Segment,
that is broken in different ways in some Linux distros and Docker images.
- at times just a hash of '', which means many machines map to the same ID
- at times a hash of the same string, when running in a Docker container

This is a replacement for _get_machine_id() that doesn't try to use `sudo`
when there is no machine-id file, because it isn't available in all enviroments
and is a bad break in the user flow even when it is usable.

We'll track multiple IDs in Segment for a few months after which
we'll drop the others. The goal is to determine a ratio between them
that we can use to normalize our metrics.

"""

machine_id = str(uuid.getnode())
if os.path.isfile(_ETC_MACHINE_ID_PATH):
with open(_ETC_MACHINE_ID_PATH, "r") as f:
machine_id = f.read()

elif os.path.isfile(_DBUS_MACHINE_ID_PATH):
with open(_DBUS_MACHINE_ID_PATH, "r") as f:
machine_id = f.read()

return machine_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

All looks great! Might be nice to break out these file paths into constants cause I see them around everywhere. Might help avoid an accidental misspelling.

That being said, this is very straightforward and might not need to be over DRYed.

Expand Down Expand Up @@ -86,6 +119,9 @@ def __init__(self):
self.installation_id_v2 = str(
uuid.uuid5(uuid.NAMESPACE_DNS, _get_stable_random_id())
)
self.installation_id_v3 = str(
uuid.uuid5(uuid.NAMESPACE_DNS, _get_machine_id_v3())
)

@property
def installation_id(self):
Expand Down
1 change: 1 addition & 0 deletions lib/streamlit/report_session.py
Expand Up @@ -401,6 +401,7 @@ def _enqueue_new_report_message(self):
imsg.user_info.installation_id = Installation.instance().installation_id
imsg.user_info.installation_id_v1 = Installation.instance().installation_id_v1
imsg.user_info.installation_id_v2 = Installation.instance().installation_id_v2
imsg.user_info.installation_id_v3 = Installation.instance().installation_id_v3
if Credentials.get_current().activation:
imsg.user_info.email = Credentials.get_current().activation.email
else:
Expand Down
44 changes: 44 additions & 0 deletions lib/tests/streamlit/metrics_util_test.py
Expand Up @@ -75,6 +75,50 @@ def test_machine_id_from_node(self):
machine_id = metrics_util._get_machine_id()
self.assertEqual(machine_id, MAC)

def test_machine_id_v3_from_etc(self):
"""Test getting the machine id from /etc"""
file_data = "etc"

with patch("streamlit.metrics_util.platform.system", return_value=False), patch(
"streamlit.metrics_util.uuid.getnode", return_value=MAC
), patch(
"streamlit.metrics_util.open", mock_open(read_data=file_data), create=True
), patch(
"streamlit.metrics_util.os.path.isfile"
) as path_isfile:

path_isfile = lambda path: path == "/etc/machine-id"

machine_id = metrics_util._get_machine_id_v3()
self.assertEqual(machine_id, file_data)

def test_machine_id_v3_from_dbus(self):
"""Test getting the machine id from /var/lib/dbus"""
file_data = "dbus"

with patch("streamlit.metrics_util.platform.system", return_value=False), patch(
"streamlit.metrics_util.uuid.getnode", return_value=MAC
), patch(
"streamlit.metrics_util.open", mock_open(read_data=file_data), create=True
), patch(
"streamlit.metrics_util.os.path.isfile"
) as path_isfile:

path_isfile = lambda path: path == "/var/lib/dbus/machine-id"

machine_id = metrics_util._get_machine_id_v3()
self.assertEqual(machine_id, file_data)

def test_machine_id_v3_from_node(self):
"""Test getting the machine id as the mac address"""

with patch("streamlit.metrics_util.platform.system", return_value=False), patch(
"streamlit.metrics_util.uuid.getnode", return_value=MAC
), patch("streamlit.metrics_util.os.path.isfile", return_value=False):

machine_id = metrics_util._get_machine_id_v3()
self.assertEqual(machine_id, MAC)

@patch("streamlit.metrics_util.file_util.get_streamlit_file_path", mock_get_path)
def test_stable_id_not_exists(self):
"""Test creating a stable id """
Expand Down
1 change: 1 addition & 0 deletions proto/streamlit/proto/NewReport.proto
Expand Up @@ -83,6 +83,7 @@ message UserInfo {
string installation_id = 1;
string installation_id_v1 = 3;
string installation_id_v2 = 4;
string installation_id_v3 = 5;
string email = 2;
}

Expand Down