fix(issue-103): release management review feedback by benhepworth · Pull Request #105 · camaraproject/SessionInsights

benhepworth · 2026-07-30T04:39:00Z

Summary

This PR addresses feedback from the release management review in issue #103 (review of rc #102). It is scoped narrowly to terminology, documentation, and naming/schema-alignment fixes — it deliberately does not touch the CloudEvent/notification/MQTT architecture.

Supersedes #104, which mixed this release-management scope with the CloudEvent/MQTT refactor. Per discussion with @hdamker and the SessionInsights bi-weekly call, that PR is being split in two for easier review:

This PR — release-management fixes from Review comments for PR #102 (Release Management) #103
Follow-up PR (refactor/cloud-event-consolidation, cut from main after this merges) — consolidates the CloudEvent refactor: removes MQTT entirely (HTTP/HTTPS-only per Commonalities' current MUST-level restriction), aligns strictly with the Commonalities implicit-subscription pattern, and removes the subscription-lifecycle schemas that don't apply to an implicit-only API

#104 will be closed manually once both this PR and the follow-up are merged.

A follow-up PR removing MQTT support entirely will be coming soon (per @hdamker's feedback that MQTT is not currently permitted under Commonalities' HTTP/HTTPS-only notification requirement). This PR does not include that removal — see "Out of scope" below.

Changes

Terminology: consistently use "insights session" over "monitoring session"; replace "streaming" with "send"; replace "event" with "notification" in spec prose (implicit subscription mechanism is now called out explicitly in info.description)
Session Quality Score: added a dedicated section to info.description clarifying what the score is and how it's delivered
MetricsPayload alignment with Application Profiles API: renamed fields to match the upstream schema (e.g. RateUnitEnum, packetLossErrorRate instead of packetLoss, packetDelay/jitter instead of latency), and now $refs shared enums instead of redefining them
createSession callback: added the callbacks section referencing NotificationEvent, resolving a Spectral S-211 unused-component warning
Spectral S-313 fixes: added format/pattern/maxLength to bare type: string fields that previously had no format constraint
Test definitions: rewrote session-insights-sendMetrics.feature to match the renamed MetricsPayload fields (removed stale latency/bitrate/resolution scenarios, added minimum/maximum boundary-value scenarios), and updated Test_definitions/README.md and the User Story doc to match

Out of scope (deferred to the follow-up PR)

MQTT3/MQTT5 support, MqttSettings, MqttSession, MqttSessionRequest, SubscriptionStatus, and the subscription-lifecycle event schemas remain untouched here. Spectral will still report the pre-existing S-034 (sink HTTPS pattern) warning and two S-033 (protocol enum) hints on this PR — both non-blocking, and both expected to be resolved by the MQTT-removal work in the follow-up PR, not by this one.

Test plan

Ran the CAMARA validation orchestrator locally (yamllint, Spectral with .spectral-r4.yaml, Python checks, GPLint) against this branch — result: pass, 0 errors, 1 non-blocking warning, 2 non-blocking hints (see "Out of scope" above)
CI validation (camara-validation.yml) passes on this PR

- Fix P-027: restore canonical 'identifying-device-from-access-token' template text in info.description (two-word drift on line 70) - MetricsPayload: align field names with ApplicationProfiles API — latency→packetDelay, packetLoss→packetErrorLossRate, bitrate replaced by optional upstreamRate/downstreamRate; all use $ref to Duration/Rate - SessionQualityData: make only `score` required; all network metric fields and `resolution` are now optional - Collapse ScoreAlertData, ResolutionProposedData, ScoreAlertNotification, and ResolutionProposedNotification into the single SessionQualityData / SessionQualityNotification; resolution is free-text operator guidance, not a prescriptive enum Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…camaraproject#103 - Replace "stream KPIs" with "send metrics" throughout - Replace "monitoring session" with "insights session" throughout - Replace "events" with "notifications" where referring to CloudEvents - Replace "KPIs" with "metrics" or "network metrics" throughout - Add "# Session Notifications" section to info.description explaining implicit subscription model, notification types, and async score delivery - Remove lingering network metric fields from SessionQualityData (score and resolution only; metrics belong in MetricsPayload, app→operator) - Fix grammar error in Wi-Fi Congestion use case example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…docs Replace "stream"/"streaming" with "send"/"reporting" and "KPIs" with "metrics" across README, User Story, and Test_definitions README/feature files to align with API terminology per issue camaraproject#103 feedback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ionId - "terminate the session" → "terminate the insights session" in Key Features - applicationSessionId description now explicitly distinguishes the app's own session reference from the operator-assigned sessionId Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- metricsTopic description: "KPI messages" → "session metrics" - MetricsPayload schema description: "KPI message" → "Application-observed session metrics" - Test_definitions README: "Valid KPI ranges" → "Valid metric ranges" with updated field names Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replace prose use of 'events' / 'event types' with 'notifications' / 'notification types' throughout session-insights.yaml descriptions. Preserves: CloudEvent (CNCF standard name), CAMARA enum values, schema file references, SubscriptionEventType schema name, MAX_EVENTS_REACHED. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tion SubscriptionEventType schema and 'types' field descriptions use 'event types' per CAMARA Event Subscription and Notification Guide. Only general prose delivery language uses 'notifications'. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Define the score as 0-100 (higher = better), explain it is computed by the network operator from its own telemetry plus app-submitted metrics, and make explicit that it is delivered asynchronously via session-quality notification rather than as a response to sendSessionMetrics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…rofiles API Rename packetErrorLossRate to packetLossErrorRate (matching AP field word order) and fix RateUnitEnum casing (bps→Bps, kbps→Kbps) to match AP conventions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… fields All 11 string fields flagged by [S-313] "String has no format/pattern/enum" now carry an explicit constraint: - id in SessionBase: format: uuid (aligns with SessionId path-param type), maxLength updated to 36, example updated to a valid UUID - subscriptionId in SubscriptionEndedData/StartedData/UpdatedData: format: uuid, maxLength updated to 36 - applicationSessionId (SessionRequest + SessionBase): pattern for opaque app-level correlation IDs (alphanumeric + common punctuation) - sink in SessionBase: permissive pattern covering webhook URLs and MQTT topics - metricsTopic / qualityTopic in MqttSettings: MQTT topic path pattern - domainName in ApplicationServer: hostname character-class pattern - resolution in SessionQualityData: broad printable-text pattern Spectral passes 0 errors / 0 warnings / 0 hints locally after these changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…csPayload fields session-insights-sendMetrics.feature still referenced the pre-rename schema (latency/packetLoss/bitrate/resolution). Update to packetDelay, jitter, packetLossErrorRate, and optional upstreamRate/downstreamRate; drop resolution (moved to SessionQualityData) and the now-invalid zero-value/missing-bitrate/missing-resolution scenarios, replacing with min/max boundary scenarios that match the current field constraints. Also updates the Test_definitions README's metric field references. Addresses Herbert's camaraproject#104 review comment that the test definitions were left out of sync with the restructured MetricsPayload.

Adds a callbacks section to createSession documenting the webhook delivery contract (NotificationEvent schema, cloudevents+json content type, bearer auth via sinkCredential). Resolves the camara-discriminator-aware-unused- component Spectral warning on NotificationEvent. Also adds the notificationsBearerAuth security scheme from CAMARA_event_common.yaml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Kevsy

LGTM - understood that the second set of fixes will follow.

hdamker · 2026-07-31T10:23:11Z

#104 will be closed manually once both this PR and the follow-up are merged.

I suggest the issue #103 is meant (#104 was the superseded PR).

Currently there are some inconsistencies (e.g. line 65 refers in info.description to an event type which probably will come with the next PR in the spec). But the direction of the PR looks good to me.

benhepworth · 2026-07-31T14:26:42Z

LGTM - understood that the second set of fixes will follow.

thanks @Kevsy! PR 106 is now ready for review.

benhepworth · 2026-07-31T14:28:53Z

#104 will be closed manually once both this PR and the follow-up are merged.

I suggest the issue #103 is meant (#104 was the superseded PR).

Currently there are some inconsistencies (e.g. line 65 refers in info.description to an event type which probably will come with the next PR in the spec). But the direction of the PR looks good to me.

correct - issue 103 was the release management issue and PR 104 was superseded by PR 105. Thanks for the review and feedback @hdamker ! PR 106 gets rid of MQTT and refactors the events to be inline with implicit CAMARA Cloud Events and is now ready for review. Thanks again!

Brings in the merge of PR camaraproject#105 so main is an ancestor of this branch. No content changes: main's tree is identical to 4529d6a, which this branch already contained. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Brings in the merge of PR camaraproject#105 so main is an ancestor of this branch. No content changes: main's tree is identical to 4529d6a, which this branch already contained.

benhepworth and others added 12 commits July 29, 2026 16:06

benhepworth requested review from Kevsy and maheshc01 as code owners July 30, 2026 04:39

This was referenced Jul 30, 2026

fix: address issue #103 release review feedback #104

Closed

refactor: consolidate to HTTP-only implicit notification subscriptions #106

Merged

test: Gherkin cleanup — operationId alignment and spec-consistent error scenarios #107

Closed

Kevsy approved these changes Jul 31, 2026

View reviewed changes

benhepworth merged commit 8b95363 into camaraproject:main Jul 31, 2026
2 checks passed

Navigation Menu

Uh oh!

fix(issue-103): release management review feedback - #105

fix(issue-103): release management review feedback#105
benhepworth merged 12 commits into
camaraproject:mainfrom
cablelabs:fix/issue-103-release-management

benhepworth commented Jul 30, 2026 •

edited

Loading

Uh oh!

Kevsy left a comment

Uh oh!

hdamker commented Jul 31, 2026

Uh oh!

Uh oh!

benhepworth commented Jul 31, 2026

Uh oh!

benhepworth commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

benhepworth commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Out of scope (deferred to the follow-up PR)

Test plan

Uh oh!

Kevsy left a comment

Choose a reason for hiding this comment

Uh oh!

hdamker commented Jul 31, 2026

Uh oh!

Uh oh!

benhepworth commented Jul 31, 2026

Uh oh!

benhepworth commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benhepworth commented Jul 30, 2026 •

edited

Loading