[CLIENT-3805] Add missing app_id attribute to Cluster class for extended metrics (#864)

There is missing code coverage for returning None from the Cluster class's app_id attribute, but that is fine because it is hard to reproduce that behavior and it's documented that as_cluster.app_id can be NULL in the C client

Extra changes:

- Allow client config option app_id to be set to None.
- Docs: fix app_id and cluster_names' types to be Optional[str] since they can be set to None.
- Docs: set default value for client config's app_id and cluster_name option to None.
- Docs: Update outdated documentation for the behavior of client config's cluster_name.
- Make a standalone script for user agent test to make easier to run locally
- In user agent standalone test, also check the server received the app_id set in the Python client's client config

---------

Co-authored-by: Julian Nguyen <109386615+juliannguyen4@users.noreply.github.com>

Author: DomPeliniAerospike

.github/workflows/smoke-tests.yml

@@ -250,6 +250,7 @@ jobs:
         type:
           - sanitizer
           - dont_validate_keys
+          - lowest_supported_server_version
       fail-fast: false
     runs-on: ubuntu-22.04
     needs: build
@@ -271,7 +272,7 @@ jobs:
     - if: ${{ matrix.type == 'sanitizer' }}
       run: echo WHEEL_GH_ARTIFACT_NAME=wheel-${{ env.LOWEST_SUPPORTED_PY_VERSION }}-sanitizer >> $GITHUB_ENV
 
-    - if: ${{ matrix.type == 'dont_validate_keys' }}
+    - if: ${{ matrix.type != 'sanitizer' }}
       run: echo WHEEL_GH_ARTIFACT_NAME=wheel-${{ env.LOWEST_SUPPORTED_PY_VERSION }} >> $GITHUB_ENV
 
     - uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0
@@ -294,7 +295,7 @@ jobs:
     - uses: ./.github/actions/run-ee-server
       with:
         registry-name: ${{ env.REGISTRY_NAME }}
-        server-tag: ${{ env.SERVER_TAG }}
+        server-tag: ${{ matrix.type == 'lowest_supported_server_version' && vars.LOWEST_SUPPORTED_SERVER_VERSION || env.SERVER_TAG }}
         registry-username: ${{ env.REGISTRY_NAME == 'docker.io' && secrets.DOCKER_HUB_BOT_USERNAME || secrets.QE_DOCKER_REGISTRY_USERNAME }}
         registry-password: ${{ env.REGISTRY_NAME == 'docker.io' && secrets.DOCKER_HUB_BOT_PW || secrets.QE_DOCKER_REGISTRY_PASSWORD }}
 
@@ -447,15 +448,16 @@ jobs:
     - run: python3 test_${{ matrix.suffix }}.py
       working-directory: test/standalone
 
-  test-misc:
+  # This is an e2e test to check that the user agent was sent correctly to the server
+  test-user-agent:
     needs: build
     strategy:
       fail-fast: false
       matrix:
-        test-case:
-          - "lowest_supported_server_version"
-          - "user_agent_with_ce"
-          - "user_agent_with_ee"
+        test-script-args:
+          - "false"
+          - "true"
+          - "true my_app_id"
     runs-on: ubuntu-22.04
     steps:
     - name: Harden the runner (Audit all outbound calls)
@@ -479,18 +481,8 @@ jobs:
         username: ${{ env.REGISTRY_NAME == 'docker.io' && secrets.DOCKER_HUB_BOT_USERNAME || secrets.QE_DOCKER_REGISTRY_USERNAME }}
         password: ${{ env.REGISTRY_NAME == 'docker.io' && secrets.DOCKER_HUB_BOT_PW || secrets.QE_DOCKER_REGISTRY_PASSWORD }}
 
-    - name: Run Aerospike server
-      if: ${{ matrix.test-case != 'user_agent_with_ee' }}
-      run: docker run -d --name aerospike -p 3000:3000 -e DEFAULT_TTL=2592000 ${{ env.REGISTRY_NAME }}/aerospike/aerospike-server:${{ env.SERVER_TAG }}
-
-    - uses: ./.github/actions/wait-for-ce-server-to-start
-      if: ${{ matrix.test-case != 'user_agent_with_ee' }}
-      with:
-        container-name: aerospike
-
-    - id: run-ee-server
-      uses: ./.github/actions/run-ee-server
-      if: ${{ matrix.test-case == 'user_agent_with_ee' }}
+    - uses: ./.github/actions/run-ee-server
+      if: ${{ startsWith(matrix.test-script-args, 'true') }}
       with:
         registry-name: ${{ env.REGISTRY_NAME }}
         server-tag: ${{ env.SERVER_TAG }}
@@ -499,77 +491,18 @@ jobs:
         # The user agent can also send the client's username if app-id is not set by the client
         env-vars: 'SECURITY=1'
 
-    - name: Install test dependencies
-      if: ${{ matrix.test-case == 'lowest_supported_server_version' }}
-      run: pip install -r test/requirements.txt
-
-    - name: Create config.conf
-      if: ${{ matrix.test-case == 'lowest_supported_server_version' }}
-      run: cp config.conf.template config.conf
-      working-directory: test
+    - if: ${{ startsWith(matrix.test-script-args, 'false') }}
+      run: docker run -d -p 3000:3000 --name aerospike -e DEFAULT_TTL=2592000 ${{ env.REGISTRY_NAME }}/aerospike/aerospike-server:${{ env.SERVER_TAG }}
 
-    - name: Run tests
-      if: ${{ matrix.test-case == 'lowest_supported_server_version' }}
-      run: python -m pytest ./new_tests
-      working-directory: test
+    - if: ${{ startsWith(matrix.test-script-args, 'false') }}
+      uses: ./.github/actions/wait-for-ce-server-to-start
+      with:
+        container-name: aerospike
 
-    - if: ${{ startsWith(matrix.test-case, 'user_agent') }}
       # Even for server versions < 8.1 that don't support user agent, this client version should still work on older servers
-      name: Run client in background
-      run: |
-        set -x
-        cat <<-EOF >> run-client-in-bg.py
-        import aerospike
-        import time
-        import sys
-        config = {
-          "hosts": [
-            ("127.0.0.1", 3000)
-          ]
-        }
-        if "${{ endsWith(matrix.test-case, 'with_ee') }}" == "true":
-          config["user"] = "superuser"
-          config["password"] = "superuser"
-        client = aerospike.client(config)
-        while True:
-          time.sleep(1)
-        EOF
-        # This shell will be closed once this step completes
-        nohup python3 run-client-in-bg.py &
-        # TODO: We want to check if the python script raises a syntax error or not. (should fail immediately)
-        # When background processes fail, this step won't fail.
-
-    - if: ${{ startsWith(matrix.test-case, 'user_agent') }}
-      name: Confirm that the user agent shows the correct client language and version (only for server 8.1)
-      run: |
-        if [[ "${{ endsWith(matrix.test-case, 'with_ee') }}" == "true" ]]; then
-          CREDENTIALS="-U superuser -P superuser"
-        fi
-
-        server_version=$(docker run --network host aerospike/aerospike-tools asinfo $CREDENTIALS -v "build")
-        major_num=$(echo $server_version | cut -d '.' -f 1)
-        minor_num=$(echo $server_version | cut -d '.' -f 2)
-        if [[ $major_num -lt 8 || ( $major_num -eq 8 && $minor_num -lt 1 ) ]]; then
-          exit 0
-        fi
-
-        info_cmd_response=$(docker run --network host aerospike/aerospike-tools asinfo $CREDENTIALS -v "user-agents" | head -n 1)
-        echo "Info command response: $info_cmd_response"
-        # Response format: user-agent=<base64 encoded string>:...
-        user_agent_base64_encoded=$(echo $info_cmd_response | perl -n -E 'say $1 if m/user-agent= ([a-zA-Z0-9+\/=]+) :/x')
-        echo $user_agent_base64_encoded
-        user_agent=$(echo $user_agent_base64_encoded | base64 -d)
-        echo $user_agent
-        # TODO: combine into one regex
-        # User agent format: <format-version>,<client language>-<version>,...
-        client_language=$(echo $user_agent | perl -n -E 'say $1 if m/[0-9]+, ([a-z]+) -/x')
-        client_version=$(echo $user_agent | perl -n -E 'say $1 if m/[0-9]+,[a-z]+- ([0-9.a-zA-Z+]+),/x')
-        echo $client_language
-        echo $client_version
-
-        test "$client_language" = "python"
-
-        # Client version from user agent
-        expected_client_version=$(python3 -m pip show aerospike | grep -i version | cut -d " " -f 2-)
-        test "$client_version" = "$expected_client_version"
-      shell: bash
+    - name: Run client in background
+      run: ./test-user-agent-e2e.bash ${{ matrix.test-script-args }}
+      working-directory: test/standalone
+
+    - if: ${{ !cancelled() }}
+      run: docker logs aerospike

aerospike_helpers/metrics/__init__.py

@@ -105,6 +105,8 @@ class Cluster:
 
     Attributes:
         cluster_name (Optional[str]): Expected cluster name for all nodes. May be :py:obj:`None`.
+        app_id (str): Application identifier. Will be set to the client's username if not set to a string in the client
+            config's ``app_id`` option. If the client does not have a username, this will be set to ``not-set``.
         invalid_node_count (int): Count of add node failures in the most recent cluster tend iteration.
         command_count (int): Command count. The value is cumulative and not reset per metrics interval.
         retry_count (int): Command retry count. There can be multiple retries for a single command.

doc/aerospike.rst

@@ -767,10 +767,20 @@ Only the `hosts` key is required; the rest of the keys are optional.
             Compress data for transmission if the object size is greater than a given number of bytes
 
             Default: ``0``, meaning 'never compress'
-        * **cluster_name** (:class:`str`)
-            Only server nodes matching this name will be used when determining the cluster name.
-        * **app_id** (:class:`str`)
+        * **cluster_name** (:class:`Optional[str]`)
+            Expected cluster name. If set to a string value, the ``cluster_name`` must match the cluster-name field
+            in the service section in each server configuration. This ensures that the specified
+            seed nodes belong to the expected cluster on startup. If not, the client will refuse
+            to add the node to the client's view of the cluster.
+
+            Default: :py:obj:`None`
+        * **app_id** (:class:`Optional[str]`)
             Application identifier.
+
+            If this is set to :py:obj:`None`, this is set to the client's username by default. If client doesn't have a username,
+            this is set to ``not-set``.
+
+            Default: :py:obj:`None`
         * **rack_id** (:class:`int`)
             Rack id where this client instance resides.
 

src/main/client/type.c

@@ -1220,7 +1220,7 @@ static int AerospikeClient_Type_Init(AerospikeClient *self, PyObject *args,
 
     PyObject *py_app_id = NULL;
     retval = PyDict_GetItemStringRef(py_config, "app_id", &py_app_id);
-    if (retval == 1) {
+    if (retval == 1 && !Py_IsNone(py_app_id)) {
         const char *str = convert_pyobject_to_str(py_app_id);
         if (!str) {
             Py_DECREF(py_app_id);

src/main/conversions.c

@@ -1054,6 +1054,24 @@ PyObject *create_py_cluster_from_as_cluster(as_error *error_p,
         PyObject_SetAttrString(py_cluster, "cluster_name", Py_None);
     }
 
+    // App Id is optional (declared in client config)
+    PyObject *py_app_id = NULL;
+    if (cluster->app_id) {
+        py_app_id = PyUnicode_FromString(cluster->app_id);
+        if (!py_app_id) {
+            goto error;
+        }
+    }
+    else {
+        py_app_id = Py_NewRef(Py_None);
+    }
+
+    int retval = PyObject_SetAttrString(py_cluster, "app_id", py_app_id);
+    Py_DECREF(py_app_id);
+    if (retval == -1) {
+        goto error;
+    }
+
     PyObject *py_invalid_node_count =
         PyLong_FromUnsignedLong(cluster->invalid_node_count);
     PyObject_SetAttrString(py_cluster, "invalid_node_count",

test/new_tests/test_connect.py

@@ -7,6 +7,7 @@
 from aerospike import exception as e
 
 import aerospike
+from contextlib import nullcontext
 
 
 @contextmanager
@@ -145,18 +146,28 @@ def test_connect_positive_shm_not_enabled(self):
             assert client.is_connected()
             assert client.shm_key() is None
 
-    def test_connect_positive_cluster_name(self):
+    @pytest.mark.parametrize(
+        "cluster_name",
+        [
+            None,
+            # This test case is for code coverage purposes
+            "invalid-cluster-name"
+        ]
+    )
+    def test_connect_with_cluster_name(self, cluster_name):
         """
-        Invoke connect() giving a cluster name. This is just a usage test (doesn't care if the server's cluster name
-        matches or not)
+        Invoke connect() giving a cluster name
         """
         config = self.connection_config.copy()
-        config["cluster_name"] = "test-cluster"
+        config["cluster_name"] = cluster_name
 
-        try:
-            self.client = aerospike.client(config).connect()
-        except e.ClientError:
-            pass
+        if cluster_name is None:
+            cm = nullcontext()
+        else:
+            cm = pytest.raises(e.ClientError)
+
+        with cm:
+            self.client = aerospike.client(config)
 
     def test_connect_positive_reconnect(self):
         """

test/new_tests/test_metrics.py

@@ -7,6 +7,8 @@
 import time
 from typing import Optional
 import re
+import aerospike
+from .test_base_class import TestBaseClass
 from importlib.metadata import version
 
 # Flags for testing callbacks
@@ -160,7 +162,19 @@ def test_metrics_writer(self):
             for item in metrics_log_filenames:
                 os.remove(item)
 
-    def test_setting_metrics_policy_custom_settings(self):
+    @pytest.fixture(scope="function", params=[None, "my_app"])
+    def get_client_and_app_id(self, request):
+        config = TestBaseClass.get_connection_config()
+        config["app_id"] = request.param
+        client = aerospike.client(config)
+
+        yield request.param, client
+
+        client.close()
+
+    def test_setting_metrics_policy_custom_settings(self, get_client_and_app_id):
+        app_id, client = get_client_and_app_id
+
         self.metrics_log_folder = "./metrics-logs"
 
         # Save bucket count for testing later
@@ -175,9 +189,9 @@ def test_setting_metrics_policy_custom_settings(self):
             labels={"a": "b"},
         )
 
-        self.as_connection.enable_metrics(policy=policy)
+        client.enable_metrics(policy=policy)
         time.sleep(3)
-        self.as_connection.disable_metrics()
+        client.disable_metrics()
 
         # These callbacks should've been called
         assert enable_triggered is True
@@ -194,6 +208,14 @@ def test_setting_metrics_policy_custom_settings(self):
             assert type(cluster.command_count) == int
             assert type(cluster.retry_count) == int
             assert type(cluster.nodes) == list
+            if type(app_id) == str:
+                assert cluster.app_id == app_id
+            elif TestBaseClass.auth_in_use():
+                # Or username if the app_id is not set
+                assert cluster.app_id == TestBaseClass.user
+            else:
+                assert cluster.app_id == "not-set"
+
             # Also check the Node and ConnectionStats objects in the Cluster object were populated
             for node in cluster.nodes:
                 assert type(node) == Node

test/standalone/run-client-in-bg.py

@@ -0,0 +1,17 @@
+import aerospike
+import time
+import sys
+config = {
+    "hosts": [
+        ("127.0.0.1", 3000)
+    ]
+}
+print(config)
+if sys.argv[1] == "true":
+    config["user"] = "superuser"
+    config["password"] = "superuser"
+if len(sys.argv) == 3:
+    config["app_id"] = sys.argv[2]
+client = aerospike.client(config)
+while True:
+    time.sleep(1)