Azure / apiops

APIOps applies the concepts of GitOps and DevOps to API deployment. By using practices from these two methodologies, APIOps can enable everyone involved in the lifecycle of API design, development, and deployment with self-service and automated tools to ensure the quality of the specifications and APIs that they’re building.
https://azure.github.io/apiops
MIT License
328 stars 193 forks source link

[BUG] Publisher: API Diagnostic are not getting overridden based on configuration.<environment>.yaml #627

Closed jaliyaudagedara closed 3 months ago

jaliyaudagedara commented 3 months ago

Release version

APIOps Toolkit for Azure APIM v6.0.1

Describe the bug

In the publisher, we can override environment specific values using configuration.<environment>.yaml file.

Supported Scenarios specifies, API Diagnostic (Provides operations for managing Diagnostic settings for the logger in an API) is supported.

And in the sample configuration.prod.yaml, we have this:

apis:
  - name: demo-conference-api
    diagnostics:
      - name: applicationinsights
        properties:
          verbosity: Error
          loggerId: "/subscriptions/[subscription Guid goes here]/resourceGroups/rg-apim-lab-prod/providers/Microsoft.ApiManagement/service/apim-prod-wk-05102022/loggers/[application insights name you chose for the destination environment]"

However it doesn't seem to be working.

In Azure Portal when we enable Diagnostic Logs for an API, it does the following.

curl 'https://management.azure.com/batch?api-version=2016-07-01' \
  --data-raw $'{  
    "requests": [
        {
            "httpMethod": "PUT",
            "relativeUrl": "/subscriptions/<subscriptionId>/resourceGroups/<resourceGroupName>/providers/Microsoft.ApiManagement/service/<apimName>/apis/<apiName>/diagnostics/<diagnosticName>?api-version=2022-09-01-preview",
            "content": {
                "id": "",
                "name": "<diagnosticName>",
                "properties": {
                    "loggerId": "/subscriptions/<subscriptionId>/resourceGroups/<resourceGroupName>/providers/Microsoft.ApiManagement/service/<apimName>/loggers/<loggerName>",
                    "verbosity": "information",
                    // Omitted for brevity
                }
            }
        }
    ]
  }'

I inspected the HTTP calls the publisher is making, and I am not seeing any HTTP call to .../apis/<apiName>/diagnostics/*. Api Diagnostic - Create Or Update

Expected behavior

The publisher should honor and override API diagnostics as specified in the following format.

apis:
  - name: demo-conference-api
    diagnostics:
      - name: applicationinsights
        properties:
          verbosity: Error
          loggerId: "/subscriptions/[subscription Guid goes here]/resourceGroups/rg-apim-lab-prod/providers/Microsoft.ApiManagement/service/apim-prod-wk-05102022/loggers/[application insights name you chose for the destination environment]"

Actual behavior

The publisher doesn't override API diagnostics.

Reproduction Steps

  1. From the source APIM, create a test HTTP API, and enable Diagnostics Logs
  2. Create a configuration.<environment>.yaml for the target APIM. Update apis.diagnostics with environment specific values
  3. Run publisher and ensure in the target APIM, Diagnostics Log are enabled as per the configuration.<environment>.yaml
github-actions[bot] commented 3 months ago
  Thank you for opening this issue! Please be patient while we will look into it and get back to you as this is an open source project. In the meantime make sure you take a look at the [closed issues](https://github.com/Azure/apiops/issues?q=is%3Aissue+is%3Aclosed) in case your question has already been answered. Don't forget to provide any additional information if needed (e.g. scrubbed logs, detailed feature requests,etc.).
  Whenever it's feasible, please don't hesitate to send a Pull Request (PR) our way. We'd greatly appreciate it, and we'll gladly assess and incorporate your changes.
jaliyaudagedara commented 3 months ago

@guythetechie, @waelkdouh,

Seems ApiDiagnostic.cs was dropped in version >6.0.0? Any particular reason?

jaliyaudagedara commented 3 months ago

This seems related: #616

guythetechie commented 3 months ago

Closing in favor of #616. We're adding support back in, accidentally removed it v6.

jaliyaudagedara commented 3 months ago

@guythetechie

Thanks for confirmation!

jaliyaudagedara commented 3 months ago

@guythetechie

Can confirm diagnostics are getting published with v6.0.1.1

spk2piyu commented 2 months ago

The version v6.0.1.1 supports extraction for api level diagnostics but not replacing loggerId from the configuration..yaml file.

Here is the APIOPS logs for API level diagnostic. Starting request 2024-09-02T11:25:48.7661199Z Method: PUT 2024-09-02T11:25:48.7662007Z Uri: https://management.azure.com/subscriptions//resourceGroups//providers/Microsoft.ApiManagement/service//apis//diagnostics/applicationinsights?api-version=2023-09-01-preview 2024-09-02T11:25:48.7663677Z Content: {"properties":{"loggerId":"/subscriptions//resourceGroups/<dev RG - it should be target rg>/providers/Microsoft.ApiManagement/service//loggers/","alwaysLog":"allErrors","backend":{"request":{"body":{"bytes":0},"headers":[]},"response":{"body":{"bytes":0},"headers":[]}},"frontend":{"request":{"body":{"bytes":0},"headers":[]},"response":{"body":{"bytes":0},"headers":[]}},"httpCorrelationProtocol":"Legacy","logClientIp":true,"metrics":true,"sampling":{"percentage":100,"samplingType":"fixed"},"verbosity":"verbose"}}

spk2piyu commented 2 months ago

@guythetechie

Can confirm diagnostics are getting published with v6.0.1.1

Are you able to replace loggerId for the uat and prod (target APIM) from publisher configuration..yaml ?? In my case, it is creating the loggerId with the same name as dev.

Starting request 2024-09-02T11:25:48.7661199Z Method: PUT 2024-09-02T11:25:48.7662007Z Uri: https://management.azure.com/subscriptions//resourceGroups//providers/Microsoft.ApiManagement/service//apis//diagnostics/applicationinsights?api-version=2023-09-01-preview 2024-09-02T11:25:48.7663677Z Content: {"properties":{"loggerId":"/subscriptions//resourceGroups/<dev RG - it should be target rg>/providers/Microsoft.ApiManagement/service//loggers/","alwaysLog":"allErrors","backend":{"request":{"body":{"bytes":0},"headers":[]},"response":{"body":{"bytes":0},"headers":[]}},"frontend":{"request":{"body":{"bytes":0},"headers":[]},"response":{"body":{"bytes":0},"headers":[]}},"httpCorrelationProtocol":"Legacy","logClientIp":true,"metrics":true,"sampling":{"percentage":100,"samplingType":"fixed"},"verbosity":"verbose"}}

jeroenmaes commented 2 months ago

The version v6.0.1.1 supports extraction for api level diagnostics but not replacing loggerId from the configuration..yaml file.

Here is the APIOPS logs for API level diagnostic. Starting request 2024-09-02T11:25:48.7661199Z Method: PUT 2024-09-02T11:25:48.7662007Z Uri: https://management.azure.com/subscriptions//resourceGroups//providers/Microsoft.ApiManagement/service//apis//diagnostics/applicationinsights?api-version=2023-09-01-preview 2024-09-02T11:25:48.7663677Z Content: {"properties":{"loggerId":"/subscriptions//resourceGroups/<dev RG - it should be target rg>/providers/Microsoft.ApiManagement/service//loggers/","alwaysLog":"allErrors","backend":{"request":{"body":{"bytes":0},"headers":[]},"response":{"body":{"bytes":0},"headers":[]}},"frontend":{"request":{"body":{"bytes":0},"headers":[]},"response":{"body":{"bytes":0},"headers":[]}},"httpCorrelationProtocol":"Legacy","logClientIp":true,"metrics":true,"sampling":{"percentage":100,"samplingType":"fixed"},"verbosity":"verbose"}}

I'm having the exact same experience in my environment.

guythetechie commented 2 months ago

We only support overriding top-level resources in the publisher. You can override service-level diagnostics (which are top-level), but you can't override API diagnostics. In other words, this works:

- diagnostics:
    properties:
      loggerId: abcd

But there is no support for something like this:

- apis
     apiA:
       diagnostics:
         properties:
           loggerId: abcd

We will look into adding support for the above configuration, as we'll need a solution to support overrides in workspaces. In the meantime, you have to update properties.loggerId in apis/apiA/diagnostics/diagnosticA/diagnosticInformation.json with the correct logger.

Also, I don't think the full resource ID of the logger is required in properties.loggerId. You should be able to update it with just /loggers/your-logger-name. This will make sure it works across all APIM instances. It will look for your-logger-name in the current APIM instance instead of the instance specified in /subscriptions/your-subscription/resourceGroups.../original-apim-instance.

jaliyaudagedara commented 2 months ago

Further @guythetechie s' answer,

Example override yaml,

apimServiceName: <apimServiceName>
namedValues:
  - name: application-insights-instrumentation-key
    properties:
      displayName: application-insights-instrumentation-key
      value: "<application-insights-instrumentation-key>"

loggers:
    - name: appinsights
      properties:
        loggerType: applicationInsights
        description: <description>
        resourceId: "/subscriptions/<subscription>/resourceGroups/<resourceGroup>/providers/microsoft.insights/components/<applicationInsightsInstance>"
        credentials:
          instrumentationKey: "{{application-insights-instrumentation-key}}"
        isBuffered: true

diagnostics:
   - name: applicationinsights
     properties:
       verbosity: Error
       loggerId: "/subscriptions/<subscription>/resourceGroups/<resourceGroup>/providers/Microsoft.ApiManagement/service/<apimServiceName>/loggers/appinsights"

apis:
  - name: <apiName>
    properties:
      serviceUrl: "<serviceUrl>"
    diagnostics:
      - name: applicationinsights
        properties:
          verbosity: information
          loggerId: "/subscriptions/<subscription>/resourceGroups/<resourceGroup>/providers/Microsoft.ApiManagement/service/<apimServiceName>/loggers/appinsights"
username-anonymous commented 2 months ago

Has this issue been fixed now as i am getting exactly the same error while using v6.0.1.1. Kindly provide the fix if it's working for anyone and configuration.env.yaml file if there is any change in it to make it work.

jeroenmaes commented 2 months ago

@guythetechie, I'm looking forward to an alternative solution in v6. This requested behaviour of transforming a nested loggerId worked just find in v5.

vinilka8 commented 1 month ago

We are also seeing this issue, it doesn't override in v6.0.1.1

This snippet doesn't work anymore

apis:
  - name: <apiName>
    properties:
      serviceUrl: "<serviceUrl>"
    diagnostics:
      - name: applicationinsights
        properties:
          verbosity: information
          loggerId: "/subscriptions/<subscription>/resourceGroups/<resourceGroup>/providers/Microsoft.ApiManagement/service/<apimServiceName>/loggers/appinsights"
jaliyaudagedara commented 1 month ago

@vinilka8,

Just to clarify, loggerId should follow the following template.

loggerId: "/subscriptions/<subscription>/resourceGroups/<resourceGroup>/providers/Microsoft.ApiManagement/service/<apimServiceName>/loggers/<loggerId>"
vinilka8 commented 1 month ago

/subscriptions/f5760094-......7ae94787f/resourceGroups/rg-apim-tst-cc-01/providers/Microsoft.ApiManagement/service/apim-org-tst-cc-01/loggers/appi-apim-tst-cc-01

this what we use, and it's not working

vinilka8 commented 1 month ago

we using this approach for a year, since version 4, we just migrated today from 4 to 6.0.1.1, but it start screaming with an error stating that it can't override, and failed the publisher

waelkdouh commented 1 month ago

Please publish the logs here. Make sure you scrape the logs before you publish though.

LukaszHryniewiecki commented 4 days ago

I see that there is new version released v 6.0.1.2in release log there is an info that that bug was fixed, however I still see same error diagnostics information for API's are not overwritten from config yaml file.

Example:

I'm using release version for linux in devops pipeline.