Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing properties for backend otel configuration #52

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dhontecillas
Copy link

@dhontecillas dhontecillas commented Oct 7, 2024

Fixes #51 : actually a layers property inside the backend config does not exist, what we have are some attributes organized the same way as we have it at the service level for the layers sections: a way to add static attributes and other params to the backend, but also to disable the stage.

This is config example that allows to disable_stage for the traces part of one single backend, and we add some attributes for the other part:

{
  "@comment": "Feature: OTEL Backend detailed configurations",
  "endpoint": "/user/creditcard",
  "backend": [
    {
      "host": ["{{.fake_api}}"],
      "url_pattern": "/user/1.json",
      "allow": [
        "credit_card"
      ],
          "extra_config": {
              "telemetry/opentelemetry": {
                    "backend": {
                        "traces": {
                            "static_attributes": [
                                {
                                    "key": "b_innerlayer",
                                    "value": "b__innerlayer_value" 
                                }
                            ],
                            "disable_stage": true
                        }
                    }
                 }
          }
    },
    {
      "host": ["{{.fake_api}}"],
      "url_pattern": "/hotels/1.json",
      "allow": [
        "name"
      ],
          "extra_config": {
              "telemetry/opentelemetry": {
                    "backend": {
                        "traces": {
                            "static_attributes": [
                                {
                                    "key": "c_innerlayer",
                                    "value": "c__innerlayer_value" 
                                }
                            ],
                            "disable_stage": false
                        }
                    }
                 }
          }
    }
  ]

}

See the extra_config parts above, and the result for a trace here with the attributes:

backend_attributes_set

And we can see that the call to user/1.json is not shown:

screenshot_2024_10_09__16_35_05

So, it works as documented in the official documentation, but , in what is missing from the documentation is the static_attributes property, that allows to add more attributes.

What is wrong in the official documentation, is that backend's extra_config -> telemetry/opentelemetry accepts the proxy properties. We cannot disable that at this inner level.

Also, in the endpoint section to override config, you can also have the backend section, that provides a custom config for the backends that this endpoints uses. So the backend part must be added to the documentation here: https://www.krakend.io/docs/enterprise/telemetry/opentelemetry-by-endpoint/#endpoint-override-of-metrics-and-traces

Signed-off-by: David Hontecillas <dhontecillas@gmail.com>
@dhontecillas dhontecillas marked this pull request as draft October 7, 2024 13:47
…the layers section

Signed-off-by: David Hontecillas <dhontecillas@gmail.com>
@dhontecillas
Copy link
Author

Is https://github.com/krakend/krakend-otel/blob/main/example/docker_compose/conf/krakend_back/configuration.json#L77 incorrect then?

I am going to review everything, and when it is ready, we will check that the documentation is also correct, and then I will convert the PR from draft to ready to review.

@adigiorgi-clickup
Copy link

@dhontecillas, in case it's useful, this config snippet works in KrakenD CE v2.7.2:

{
  "backend": [
    {
      "encoding": "no-op",
      "extra_config": {
        "telemetry/opentelemetry": {
          "layers": {
            "backend": {
              "metrics": {
                "static_attributes": [
                  {
                    "key": "backend_service",
                    "value": "clips-service"
                  }
                ]
              },
              "traces": {
                "static_attributes": [
                  {
                    "key": "backend_service",
                    "value": "clips-service"
                  }
                ]
              }
            }
          }
        }
      },
      "method": "GET",
      "url_pattern": "/clips-service-v3/health",
      "host": [
        "http://host.docker.internal:4042"
      ],
      "sd": "static"
    }
  ],
  "endpoint": "/data/v3/utils/health/clips_service",
  "method": "GET"
}

It correctly adds the backend_service tag to both metrics and spans:
Screenshot 2024-10-09 at 9 09 21 AM
Screenshot 2024-10-09 at 9 12 09 AM

@dhontecillas
Copy link
Author

@dhontecillas, in case it's useful, this config snippet works in KrakenD CE v2.7.2:

{
  "backend": [
    {
      "encoding": "no-op",
      "extra_config": {
        "telemetry/opentelemetry": {
          "layers": {
            "backend": {
              "metrics": {
                "static_attributes": [
                  {
                    "key": "backend_service",
                    "value": "clips-service"
                  }
                ]
              },
              "traces": {
                "static_attributes": [
                  {
                    "key": "backend_service",
                    "value": "clips-service"
                  }
                ]
              }
            }
          }
        }
      },
      "method": "GET",
      "url_pattern": "/clips-service-v3/health",
      "host": [
        "http://host.docker.internal:4042"
      ],
      "sd": "static"
    }
  ],
  "endpoint": "/data/v3/utils/health/clips_service",
  "method": "GET"
}

It correctly adds the backend_service tag to both metrics and spans: Screenshot 2024-10-09 at 9 09 21 AM Screenshot 2024-10-09 at 9 12 09 AM

This is wrong .. I mean, it might be working, but should not work this way.. I am going to review it carefully.

Please, do not rely on this working this way. For now stick with the global config only, as it is documented for the CE version:
https://www.krakend.io/docs/telemetry/opentelemetry/

As you can see, in the enterprise version, the format for the overrides is different (and already documented, so it should be the reference): https://www.krakend.io/docs/enterprise/telemetry/opentelemetry-by-endpoint/

Let me see how I fix this, but if we put those attributes at the endpoint and backend level, the naming should be consistent.

Sorry for the inconvenience.

@dhontecillas
Copy link
Author

@adigiorgi-clickup : we are going to address this issue here krakend/krakend-otel#36 (take into account the PR is just a draft). We want to have the feature for Community Edition, but also we must make it coherent with the Enterprise Edition.

@dhontecillas dhontecillas marked this pull request as ready for review October 10, 2024 11:56
@dhontecillas
Copy link
Author

@alombarte I've fixed the tests, the PR is ready to review and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Property "endpoints[*].backend[*].telemetry/opentelemetry.layers" not allowed
2 participants