Improvement: Add tracing for subscription-states

[Rutuja-Patil-Bosch]

#449

• Code updated
• UT update is pending

[Rutuja-Patil-Bosch]

The required CI workflows appear to be awaiting maintainer approval before execution. Could one of the maintainers please approve the workflow run

[castler]

@Rutuja-Patil-Bosch can you pleaes clean up the git history of this PR? To have only your changes included and no merge commits?

[castler]

Can you then please also add necessary tests for the added code? Then a more in depth review makes sense.

I will mark the PR as draft in the meantime - please feel free to mark it again ready.

[Rutuja-Patil-Bosch]

Can you then please also add necessary tests for the added code? Then a more in depth review makes sense.

I will mark the PR as draft in the meantime - please feel free to mark it again ready.

@castler : Yes, test cases are ready. I'll rebase the PR so that no merged commit will be visible & will commit test cases also

[Copilot]

Pull request overview

Adds proxy-side tracing support for subscription-state related operations so the tracing runtime can emit the configured tracepoints for state changes and subscription-state-change handler lifecycle/callbacks.

Changes:

• Added new proxy tracing entry points for subscription state change and handler register/deregister/callback tracepoints.
• Wired ProxyEventBase::{Set,Unset}SubscriptionStateChangeHandler() to emit tracing when called.
• Updated comtrace_config_schema.json to indicate LoLa support for the previously unimplemented tracepoints.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
score/mw/com/impl/tracing/proxy_event_tracing.h	Declares new tracing functions for subscription-state tracepoints.
score/mw/com/impl/tracing/proxy_event_tracing.cpp	Implements new tracepoint emitters (state changed, handler register/deregister, handler callback).
score/mw/com/impl/tracing/configuration/comtrace_config_schema.json	Removes _lola_support: false markers for subscription-state tracepoints in proxy-side config sections.
score/mw/com/impl/proxy_event_base.cpp	Calls tracing on subscription-state-change handler register/deregister.

Comments suppressed due to low confidence (1)

score/mw/com/impl/proxy_event_base.cpp:169

• The new null-check only guards the trace call, but the function still unconditionally dereferences binding_base_. This is inconsistent with the check and can still crash when the binding is invalid. Also, unlike most other ProxyEventBase APIs, this method does not delegate to proxy_event_base_mock_ when a mock is injected.

    if (binding_base_ != nullptr)
    {
        tracing::TraceUnsetSubscriptionStateChangeHandler(tracing_data_, *binding_base_);
    }
    return binding_base_->UnsetSubscriptionStateChangeHandler();

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

score/mw/com/impl/proxy_event_base.cpp:160

• The null-check around tracing does not prevent dereferencing binding_base_; if binding_base_ is ever null (e.g. moved-from object), this still crashes, and the guard makes the code look safer than it is. Either assert binding_base_ is set (consistent with other methods) or return an error when it is null.

    if (binding_base_ != nullptr)
    {
        tracing::TraceSetSubscriptionStateChangeHandler(tracing_data_, *binding_base_);
    }
    return binding_base_->SetSubscriptionStateChangeHandler(std::move(handler));

score/mw/com/impl/proxy_event_base.cpp:169

• Same as SetSubscriptionStateChangeHandler(): the tracing guard checks binding_base_ for null, but the method still unconditionally dereferences binding_base_ afterwards. Prefer an assertion (or early error return) and then trace/call the binding.

    if (binding_base_ != nullptr)
    {
        tracing::TraceUnsetSubscriptionStateChangeHandler(tracing_data_, *binding_base_);
    }
    return binding_base_->UnsetSubscriptionStateChangeHandler();

score/mw/com/impl/tracing/proxy_event_tracing.cpp:421

• TraceSubscriptionStateChanged() and TraceCallSubscriptionStateChangeHandler() are added, but there are currently no call sites for either helper in the codebase. This means the tracepoints "trace_subscription_state_changed" and "trace_subscription_state_change_handler_callback" will still never be emitted (despite being enabled in config/schema).

void TraceSubscriptionStateChanged(ProxyEventTracingData& proxy_event_tracing_data,
                                   const ProxyEventBindingBase& proxy_event_binding_base,
                                   SubscriptionState new_state) noexcept
{
    if (proxy_event_tracing_data.enable_subscription_state_changed)

score/mw/com/impl/tracing/proxy_event_tracing.cpp:420

• There are existing unit tests for other proxy event tracing helpers (e.g. TraceSubscribe/TraceUnsubscribe), but the newly added subscription-state trace helpers are not covered. Adding UTs would help validate the correct trace point selection (event vs field) and that the local data chunk contains the 1-byte SubscriptionState value for the *_changed and *_callback tracepoints.

void TraceSubscriptionStateChanged(ProxyEventTracingData& proxy_event_tracing_data,
                                   const ProxyEventBindingBase& proxy_event_binding_base,
                                   SubscriptionState new_state) noexcept
{

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (5)

score/mw/com/impl/tracing/proxy_event_tracing.cpp:560

• TraceCallSubscriptionStateChangeHandler() is implemented, but there are no call sites outside unit tests. This means the configured trace point trace_subscription_state_change_handler_callback will never fire when the user handler is invoked.

void TraceCallSubscriptionStateChangeHandler(ProxyEventTracingData& proxy_event_tracing_data,
                                             const ProxyEventBindingBase& proxy_event_binding_base,
                                             SubscriptionState new_state) noexcept
{
    if (proxy_event_tracing_data.enable_call_subscription_state_change_handler)
    {
        const auto service_element_instance_identifier =
            proxy_event_tracing_data.service_element_instance_identifier_view;
        const auto service_element_type =
            service_element_instance_identifier.service_element_identifier_view.service_element_type;

        TracingRuntime::TracePointType trace_point{};
        if (service_element_type == ServiceElementType::EVENT)
        {
            trace_point = ProxyEventTracePointType::SUBSCRIPTION_STATE_CHANGE_HANDLER_CALLBACK;
        }
        else if (service_element_type == ServiceElementType::FIELD)
        {
            trace_point = ProxyFieldTracePointType::SUBSCRIPTION_STATE_CHANGE_HANDLER_CALLBACK;
        }
        else
        {
            // Suppress "AUTOSAR C++14 M0-1-1", The rule states: "A project shall not contain unreachable code"
            // This is false positive, the enum has more fields than EVENT and FIELD so we might reach this branch.
            // coverity[autosar_cpp14_m0_1_1_violation : FALSE]
            SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(false, "Service element type must be EVENT or FIELD");
        }

        const auto binding_type = proxy_event_binding_base.GetBindingType();
        const auto trace_result =
            TraceData(service_element_instance_identifier, trace_point, binding_type, ConvertToFatPointer(new_state));
        UpdateTracingDataFromTraceResult(trace_result,
                                         proxy_event_tracing_data,
                                         proxy_event_tracing_data.enable_call_subscription_state_change_handler);
    }

score/mw/com/impl/tracing/proxy_event_tracing_test.cpp:923

• This test asserts the 8-parameter Trace overload is never called, but TraceSetSubscriptionStateChangeHandler() uses TraceData() which calls the 6-parameter overload. The expectation should be set on the 6-parameter overload (or both) to actually prevent tracing.

TEST_P(ProxyEventTraceSetSubscriptionStateChangeHandlerFixture,
       TraceSetSubscriptionStateChangeHandlerWillNotDispatchIfTracingDisabled)
{
    // Given a ProxyEventTracingData with all trace points disabled
    WithAValidProxyEventTracingData();

    // Expecting TraceData will not be called on the TracingRuntime binding
    EXPECT_CALL(tracing_runtime_mock_, Trace(_, _, _, _, _, _, _, _)).Times(0);

score/mw/com/impl/tracing/proxy_event_tracing_test.cpp:988

• This test asserts the 8-parameter Trace overload is never called, but TraceUnsetSubscriptionStateChangeHandler() calls TraceData() which uses the 6-parameter Trace overload. The expectation should target the 6-parameter overload (or both).

TEST_P(ProxyEventTraceUnsetSubscriptionStateChangeHandlerFixture,
       TraceUnsetSubscriptionStateChangeHandlerWillNotDispatchIfTracingDisabled)
{
    // Given a ProxyEventTracingData with all trace points disabled
    WithAValidProxyEventTracingData();

    // Expecting TraceData will not be called on the TracingRuntime binding
    EXPECT_CALL(tracing_runtime_mock_, Trace(_, _, _, _, _, _, _, _)).Times(0);

score/mw/com/impl/tracing/proxy_event_tracing_test.cpp:1054

• This test asserts the 8-parameter Trace overload is never called, but TraceCallSubscriptionStateChangeHandler() uses TraceData() which calls the 6-parameter overload. The expectation should be set on the 6-parameter overload (or both) to make the test effective.

TEST_P(ProxyEventTraceCallSubscriptionStateChangeHandlerFixture,
       TraceCallSubscriptionStateChangeHandlerWillNotDispatchIfTracingDisabled)
{
    // Given a ProxyEventTracingData with all trace points disabled
    WithAValidProxyEventTracingData();

    // Expecting TraceData will not be called on the TracingRuntime binding
    EXPECT_CALL(tracing_runtime_mock_, Trace(_, _, _, _, _, _, _, _)).Times(0);

score/mw/com/impl/bindings/lola/subscription_state_machine.cpp:176

• The comment says the user handler is called under state_mutex_ lock "which has always been acquired within this method", but TransitionToState() itself does not lock state_mutex_. Per subscription_state_machine.h:141-145, this method must be called with the lock already held by the caller.

    if (subscription_state_change_handler_.has_value())
    {
        // We call the user-provided handler under state_mutex_ lock, which has always been acquired within this method.
        // This is documented in the AoUs of SubscriptionStateChangeHandler!
        const auto keep_handler =
            subscription_state_change_handler_.value()(SubscriptionStateMachineStateToSubscriptionState(newState));
        if (!keep_handler)

[Rutuja-Patil-Bosch]

@castler @crimson11 : I have updated code as per finding. I have following queries

1. Should I address copilot findings?
2. The required CI workflows appear to be awaiting maintainer approval before execution. Could you please approve the workflow run

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (12)

score/mw/com/impl/tracing/proxy_event_tracing.cpp:576

• SetupSubscriptionStateChangeTracing() is never called anywhere outside unit tests/docs, so TraceSubscriptionStateChanged will never be triggered in production and the trace_subscription_state_changed tracepoint remains effectively unimplemented.

void SetupSubscriptionStateChangeTracing(ProxyEventTracingData& proxy_event_tracing_data,
                                         ProxyEventBindingBase& proxy_event_binding_base) noexcept
{
    // Create a callback that will be invoked when subscription state changes
    if (proxy_event_tracing_data.enable_subscription_state_changed)
    {
        score::cpp::callback<void(SubscriptionState), 64U> tracing_callback =
            [&proxy_event_tracing_data, &proxy_event_binding_base](SubscriptionState new_state) noexcept {
                TraceSubscriptionStateChanged(proxy_event_tracing_data, proxy_event_binding_base, new_state);
            };
        // Set callback on the binding's state machine
        proxy_event_binding_base.SetSubscriptionStateChangeTracingCallback(std::move(tracing_callback));
    }
}

score/mw/com/impl/proxy_event_base.cpp:160

• The null-check around TraceSetSubscriptionStateChangeHandler() is misleading because binding_base_ is dereferenced unconditionally immediately afterwards; if binding_base_ can be null this still crashes, and if it cannot be null the check is redundant.

    if (binding_base_ != nullptr)
    {
        tracing::TraceSetSubscriptionStateChangeHandler(tracing_data_, *binding_base_);
    }
    return binding_base_->SetSubscriptionStateChangeHandler(std::move(handler));

score/mw/com/impl/proxy_event_base.cpp:169

• Same as above for UnsetSubscriptionStateChangeHandler(): the null-check does not prevent a null dereference (the method still unconditionally dereferences binding_base_), so it either needs proper error handling/assertion or should be removed for clarity.

    if (binding_base_ != nullptr)
    {
        tracing::TraceUnsetSubscriptionStateChangeHandler(tracing_data_, *binding_base_);
    }
    return binding_base_->UnsetSubscriptionStateChangeHandler();

score/mw/com/impl/tracing/proxy_event_tracing_test.cpp:840

• This expectation uses the 8-argument TracingRuntimeMock::Trace(...) overload, but TraceSubscriptionStateChanged() goes through TraceData(...) which calls the 6-argument overload. As written, the test can still pass even if tracing occurs.

    // Expecting TraceData will not be called on the TracingRuntime binding
    EXPECT_CALL(tracing_runtime_mock_, Trace(_, _, _, _, _, _, _, _)).Times(0);

score/mw/com/impl/tracing/proxy_event_tracing_test.cpp:923

• This expectation is on the 8-argument Trace(...) overload; TraceSetSubscriptionStateChangeHandler() calls TraceData(...) which uses the 6-argument overload, so this does not actually assert that tracing is suppressed.

    // Expecting TraceData will not be called on the TracingRuntime binding
    EXPECT_CALL(tracing_runtime_mock_, Trace(_, _, _, _, _, _, _, _)).Times(0);

score/mw/com/impl/tracing/proxy_event_tracing_test.cpp:988

• This uses the wrong Trace(...) overload for the no-dispatch assertion. TraceUnsetSubscriptionStateChangeHandler() uses TraceData(...) (6 args), so the test currently doesn't fail if a trace is emitted.

    // Expecting TraceData will not be called on the TracingRuntime binding
    EXPECT_CALL(tracing_runtime_mock_, Trace(_, _, _, _, _, _, _, _)).Times(0);

score/mw/com/impl/tracing/proxy_event_tracing_test.cpp:1054

• This expectation targets the 8-argument Trace(...) overload, but TraceCallSubscriptionStateChangeHandler() emits via TraceData(...) (6-argument overload). The test can pass even if tracing is called.

    // Expecting TraceData will not be called on the TracingRuntime binding
    EXPECT_CALL(tracing_runtime_mock_, Trace(_, _, _, _, _, _, _, _)).Times(0);

score/mw/com/impl/tracing/configuration/comtrace_config_schema.json:188

• Same issue as above in the per-service-element properties block: the schema currently implies LoLa supports these tracepoints, but trace_subscription_state_changed isn't wired (no callback installed) and trace_subscription_state_change_handler_callback isn't invoked.

                    "trace_subscription_state_changed": {
                      "$ref": "#/$defs/trace_subscription_state_changed"
                    },
                    "trace_subscription_state_change_handler_registered": {
                      "$ref": "#/$defs/trace_subscription_state_change_handler_registered"
                    },
                    "trace_subscription_state_change_handler_deregistered": {
                      "$ref": "#/$defs/trace_subscription_state_change_handler_deregistered"
                    },
                    "trace_subscription_state_change_handler_callback": {
                      "$ref": "#/$defs/trace_subscription_state_change_handler_callback"
                    },

score/mw/com/design/ipc_tracing/README.md:216

• The configuration example uses enable_* keys, but the trace filter configuration keys are trace_subscription_state_* (as defined in comtrace_config_schema.json). This example should use the schema keys so users can copy/paste it into configs.

{
  "enable_subscription_state_changed": true,
  "enable_trace_subscription_state_change_handler_registered": true,
  "enable_trace_subscription_state_change_handler_deregistered": true,
  "enable_call_subscription_state_change_handler": true

score/mw/com/impl/tracing/proxy_event_tracing.cpp:530

• TraceCallSubscriptionStateChangeHandler() is implemented (and the schema now exposes trace_subscription_state_change_handler_callback), but there is no production call site for it (only unit tests). As-is, the handler-callback tracepoint will never be emitted.

void TraceCallSubscriptionStateChangeHandler(ProxyEventTracingData& proxy_event_tracing_data,
                                             const ProxyEventBindingBase& proxy_event_binding_base,
                                             SubscriptionState new_state) noexcept
{
    if (proxy_event_tracing_data.enable_call_subscription_state_change_handler)

score/mw/com/design/ipc_tracing/README.md:229

• The implementation description states that successful ProxyEvent::Subscribe() calls SetupSubscriptionStateChangeTracing(), but there is currently no production call site for SetupSubscriptionStateChangeTracing(...) (only unit tests/docs). Either add the missing call in code or adjust this section/sequence diagram so it matches actual behavior.

Unlike API call tracing, subscription state change tracing uses a different pattern because state transitions happen asynchronously within the state machine and must be captured efficiently. Instead of wrapping user callbacks (which would exceed callback capacity constraints), we use a separate 64-byte tracing callback installed at the `SubscriptionStateMachine` level:

1. When `ProxyEvent::Subscribe()` is called successfully, it calls `SetupSubscriptionStateChangeTracing()` to install the tracing callback
2. The tracing callback is invoked by `SubscriptionStateMachine::TransitionToState()` at every state change
3. The callback dispatches to `TraceSubscriptionStateChanged()`, which checks the enable flag and sends the trace to `TracingRuntime`
4. Other state change handlers (registration/unregistration) are traced directly at the point where they are called (in `ProxyEventBase`)

score/mw/com/impl/tracing/configuration/comtrace_config_schema.json:98

• _lola_support : false was removed for trace_subscription_state_changed and trace_subscription_state_change_handler_callback, but the code currently has no production call site installing the tracing callback and does not invoke the handler-callback tracepoint. The schema therefore advertises LoLa support that isn't actually present.

This issue also appears on line 177 of the same file.

                "trace_subscription_state_changed": {
                  "$ref": "#/$defs/trace_subscription_state_changed"
                },
                "trace_subscription_state_change_handler_registered": {
                  "$ref": "#/$defs/trace_subscription_state_change_handler_registered"
                },
                "trace_subscription_state_change_handler_deregistered": {
                  "$ref": "#/$defs/trace_subscription_state_change_handler_deregistered"
                },
                "trace_subscription_state_change_handler_callback": {
                  "$ref": "#/$defs/trace_subscription_state_change_handler_callback"
                },