Skip to content

OpenTelemetryTracingProvider : New configurations to export via HTTP#2998

Merged
istio-testing merged 10 commits intoistio:masterfrom
dynatrace-oss-contrib:otel-http-exporter
Jan 26, 2024
Merged

OpenTelemetryTracingProvider : New configurations to export via HTTP#2998
istio-testing merged 10 commits intoistio:masterfrom
dynatrace-oss-contrib:otel-http-exporter

Conversation

@joaopgrassi
Copy link
Member

@joaopgrassi joaopgrassi commented Nov 20, 2023

Closes #47835

This PR is an initial iteration/attempt in extending the OpenTelemetryTracingProvider in MeshConfig with new configurations that allow exporting OpenTelemetry spans via HTTP instead of gRPC.

The ability to export OTel spans via HTTP is a populate request in Envoy and support for it was added in this PR: envoyproxy/envoy#29207 - starting from Envoy v1.28.0. This is also important due to the eminent OpenTracing deprecation in Envoy, starting from v1.29.0, since there's observability back-ends that don't support OTLP/gRPC and only OTLP/HTTP.

As discussed in Slack/WG meeting I'm opening this as a starting point to see what the community thinks and decide how we can move forward.

The new configs here can be used in the following way:

  1. Add/change the OpenTelemetry extension provider in MeshConfig:
    - name: otel-tracing
      opentelemetry:
        port: 443
        service: my.olly-backend.com
        http: # >>> Here are new configs
          path: "/api/otlp/traces" # will be joined with service host name
          timeout: 10s
          headers:
          - name: "Some custom header"
            value: "some value"
  1. Then would need to add a ServiceEntry for their observability back-end
k apply -n istio-system -f - <<EOF
apiVersion: networking.istio.io/v1alpha3
kind: ServiceEntry
metadata:
  name: my-olly-backend
spec:
  hosts:
  - my.olly-backend.com
  ports:
  - number: 443
    name: https-port
    protocol: HTTPS
  resolution: DNS
  location: MESH_EXTERNAL
---
apiVersion: networking.istio.io/v1alpha3
kind: DestinationRule
metadata:
  name: my-olly-backend
spec:
  host: my.olly-backend.com
  trafficPolicy:
    portLevelSettings:
    - port:
        number: 443
      tls:
        mode: SIMPLE

Then, redeploy/restart Envoys and then, they would be configured to send OpenTelemetry spans via HTTP to my.olly-backend.com/api/otlp/traces. In the end, this will configure the http_service in Envoy.

@joaopgrassi joaopgrassi requested a review from a team as a code owner November 20, 2023 15:10
@istio-policy-bot
Copy link

😊 Welcome @joaopgrassi! This is either your first contribution to the Istio api repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test labels Nov 20, 2023
@istio-testing
Copy link
Collaborator

Hi @joaopgrassi. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zirain
Copy link
Member

zirain commented Nov 20, 2023

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Nov 20, 2023
Copy link
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move service and port in HTTPService, but not sure how to make it compatible

@zirain
Copy link
Member

zirain commented Nov 24, 2023

We can move service and port in HTTPService, but not sure how to make it compatible

you cannot assume it's HTTP.

@zirain
Copy link
Member

zirain commented Jan 24, 2024

@howardjohn @costinm I think this PR is good to go, do you have futher concen?

At least, we should make decesion which one is the right direction, this or #3002 or #3003?

Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like to see some examples to understand how to use this

@joaopgrassi
Copy link
Member Author

joaopgrassi commented Jan 25, 2024

would like to see some examples to understand how to use this

@linsun You mean how users would configure Istio with this? I can give some example:

  1. Would need to add/change the OpenTelemetry extension provider in MeshConfig:
    - name: otel-tracing
      opentelemetry:
        port: 443
        service: my.olly-backend.com
        otlp_http: # >>> Here are new configs
          path: "/api/otlp/traces"
          timeout: 10s
          headers:
          - name: "Some custom header"
            value: "some value"
  1. Then would need to add a ServiceEntry for their observability back-end
k apply -n istio-system -f - <<EOF
apiVersion: networking.istio.io/v1alpha3
kind: ServiceEntry
metadata:
  name: my-olly-backend
spec:
  hosts:
  - my.olly-backend.com
  ports:
  - number: 443
    name: https-port
    protocol: HTTPS
  resolution: DNS
  location: MESH_EXTERNAL
---
apiVersion: networking.istio.io/v1alpha3
kind: DestinationRule
metadata:
  name: my-olly-backend
spec:
  host: my.olly-backend.com
  trafficPolicy:
    portLevelSettings:
    - port:
        number: 443
      tls:
        mode: SIMPLE

Then, redeploy/restart Envoys and then, they would be configured to send OpenTelemetry spans via HTTP to my.olly-backend.com/api/otlp/traces. In the end, this will configure the http_service in Envoy.

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one minor name change

@linsun
Copy link
Member

linsun commented Jan 25, 2024

would like to see some examples to understand how to use this

@linsun You mean how users would configure Istio with this? I can give some example:

1. Would need to add/change the OpenTelemetry extension provider in `MeshConfig`:
    - name: otel-tracing
      opentelemetry:
        port: 443
        service: my.olly-backend.com
        otlp_http: # >>> Here are new configs
          path: "/api/otlp/traces"
          timeout: 10s
          headers:
          - name: "Some custom header"
            value: "some value"
2. Then would need to add a `ServiceEntry` for their observability back-end
k apply -n istio-system -f - <<EOF
apiVersion: networking.istio.io/v1alpha3
kind: ServiceEntry
metadata:
  name: my-olly-backend
spec:
  hosts:
  - my.olly-backend.com
  ports:
  - number: 443
    name: https-port
    protocol: HTTPS
  resolution: DNS
  location: MESH_EXTERNAL
---
apiVersion: networking.istio.io/v1alpha3
kind: DestinationRule
metadata:
  name: my-olly-backend
spec:
  host: my.olly-backend.com
  trafficPolicy:
    portLevelSettings:
    - port:
        number: 443
      tls:
        mode: SIMPLE

Then, redeploy/restart Envoys and then, they would be configured to send OpenTelemetry spans via HTTP to my.olly-backend.com/api/otlp/traces. In the end, this will configure the http_service in Envoy.

Yep, could we have some of these in the API comment in this PR? Not everyone can ping you for usage like me in github :)

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 25, 2024
Copy link
Member

@hanxiaop hanxiaop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the right direction.

@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 26, 2024
@joaopgrassi
Copy link
Member Author

joaopgrassi commented Jan 26, 2024

Yep, could we have some of these in the API comment in this PR? Not everyone can ping you for usage like me in github :)

@linsun you mean add this to the PR description? If so, I added it now (also to the linked issue). My plan though is to add documentation with a full example when the feature is complete in Istio.

Edit: Never mind, I added now to the proto file, so the examples should show in the generated html file.

@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 26, 2024
@linsun
Copy link
Member

linsun commented Jan 26, 2024

Yep, could we have some of these in the API comment in this PR? Not everyone can ping you for usage like me in github :)

@linsun you mean add this to the PR description? If so, I added it now (also to the linked issue). My plan though is to add documentation with a full example when the feature is complete in Istio.

Edit: Never mind, I added now to the proto file, so the examples should show in the generated html file.

Thank you! Yep most of times, I'm looking at proto and generated html to understand how to use things. The example you added was helpful!

@linsun
Copy link
Member

linsun commented Feb 7, 2024

Per @zirain's req, we discussed the request to cherry pick this to Istio 1.21 branch in today's istio WG meeting. The general consensus is all api changes will require TOC approval to cherry pick if this misses the release branch cutting. Given there is still impl PR open on #3002 and we don't see a strong reason to make an exception for cherry pick this API change, we prefer to not cherry pick and wait till it lands in istio 1.22.

@linsun
Copy link
Member

linsun commented Feb 7, 2024

cc @istio/release-managers-1-21 ^^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenTelemetryTracingProvider: Allow exporting traces via HTTP

8 participants