5G-MAG / rt-5gms-application-function

5G Media Streaming - Application Function
https://www.5g-mag.com/streaming
Other
11 stars 6 forks source link

M1 | Metrics Provisioning Configuration OpenAPI models #134

Closed stojkovicv closed 9 months ago

stojkovicv commented 9 months ago

I am opening this thread to address potential bug mentioned in the comment of the M1 Metrics Implementation discussion. The main issue here is the way on how to create Metrics Configuration ID. External OpenAPI function msaf_api_metrics_reporting_configuration_create() is declared in the way that it requires ID as an argument from the Create operation. On the other hand, there is an approved way to generate this ID within AF.

rjb1000 commented 9 months ago

The fundamental problem is that MetricsReportingConfiguration.metricsReportingConfigurationId is marked as required in the latest Rel-17 YAML, but isn't (yet) marked as readOnly to indicate that the value is assigned by the 5GMS AF.

As an interim measure, this can be taken care of by adding a suitable override file here:

rjb1000 commented 9 months ago

The secondary problem is that the business logic in the 5GMS AF reference implementation appears to be accepting MetricsReportingConfiguration.metricsReportingConfigurationId as an input value and storing it. This is incorrect. The code needs to ignore this value if provided as input.

davidjwbbc commented 9 months ago

Previously, to work around this issue, we wrote our own parser for the create and update operations which ignored the ID value. With the change to the openapi-generator templates and the introduction of read-only flags to some of the TS 26.512 OpenAPI YAML files, the generated object parser will ignore read-only fields on request parsing.

Adding the read-only flags to a version of the OpenAPI YAML file in the overrides is the best way to handle this now.

rjb1000 commented 9 months ago

Adding the read-only flags to a version of the OpenAPI YAML file in the overrides is the best way to handle this now.

OK. Great. So who is going to make this happen?

dsilhavy commented 9 months ago

@stojkovicv Please take care of this and create an override file here: https://github.com/5G-MAG/rt-common-shared/tree/main/5gms/5G_APIs-overrides. This change complements your implementation of M1 QoE Metrics Reporting.

stojkovicv commented 9 months ago

The TS 26.512 V17.6.0 M1 Metrics yaml configuration file seems to be correct, because for the Create operation, where ID is not needed - the path field was already specified to not require metrics ID. That seems correct. However all 3 (or 4 with patch) other operations have different path, where ID is required indeed.

I've setted readOnly to true for components for metrics ID, with latest PR

dsilhavy commented 9 months ago

@stojkovicv @davidjwbbc Can we close this issue as https://github.com/5G-MAG/rt-common-shared/pull/30 has been merged?

davidjwbbc commented 9 months ago

@stojkovicv @davidjwbbc Can we close this issue as 5G-MAG/rt-common-shared#30 has been merged?

Yes, this can be closed with the PR merge.

davidjwbbc commented 8 months ago

5G-MAG/Standards#102 notes that the metricsReportigConfigurationId field is not marked as read-only in the OpenAPI YAML.