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 tracing support #5043

Merged
merged 12 commits into from
Apr 13, 2023
Merged

Add tracing support #5043

merged 12 commits into from
Apr 13, 2023

Conversation

yangyy93
Copy link
Member

@yangyy93 yangyy93 commented Feb 2, 2023

Add support for exporting tracing data to opentelemetry
close #399

Signed-off-by: yy [[email protected]]

@yangyy93 yangyy93 changed the title Add tracing support WIP: Add tracing support Feb 2, 2023
@yangyy93 yangyy93 force-pushed the tracing branch 4 times, most recently from 3936594 to 316f586 Compare February 16, 2023 04:04
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #5043 (98dcc35) into main (cc3b6f7) will decrease coverage by 0.14%.
The diff coverage is 63.67%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5043      +/-   ##
==========================================
- Coverage   77.85%   77.71%   -0.14%     
==========================================
  Files         138      139       +1     
  Lines       17759    18048     +289     
==========================================
+ Hits        13826    14026     +200     
- Misses       3665     3750      +85     
- Partials      268      272       +4     
Impacted Files Coverage Δ
cmd/contour/serve.go 20.25% <0.00%> (-1.53%) ⬇️
internal/xdscache/v3/listener.go 89.75% <66.66%> (-2.38%) ⬇️
pkg/config/parameters.go 87.37% <90.62%> (+0.39%) ⬆️
internal/envoy/v3/tracing.go 93.33% <93.33%> (ø)
cmd/contour/servecontext.go 83.02% <100.00%> (+0.95%) ⬆️
internal/envoy/v3/listener.go 98.42% <100.00%> (+0.01%) ⬆️

... and 4 files with indirect coverage changes

@yangyy93 yangyy93 changed the title WIP: Add tracing support Add tracing support Feb 16, 2023
@yangyy93 yangyy93 marked this pull request as ready for review February 16, 2023 07:12
@yangyy93 yangyy93 requested a review from a team as a code owner February 16, 2023 07:12
@yangyy93 yangyy93 requested review from stevesloka and skriss and removed request for a team February 16, 2023 07:12
@skriss skriss added the release-note/major A major change that needs more than a paragraph of explanation in the release notes. label Feb 16, 2023
@yangyy93
Copy link
Member Author

@skriss any suggestions?

@pavelnikolov
Copy link

What is remaining to be done before this PR can be merged?

@pavelnikolov
Copy link

@yangyy93 it looks like a changelog file is missing:

Run go run ./hack/actions/check-changefile-exists.go
go: downloading github.com/google/go-github/v48 v48.2.0
go: downloading github.com/sirupsen/logrus v1.[9](https://github.com/projectcontour/contour/actions/runs/4382443387/jobs/7671496430#step:5:10).0
go: downloading golang.org/x/oauth2 v0.6.0
go: downloading golang.org/x/sys v0.6.0
go: downloading github.com/google/go-querystring v1.1.0
go: downloading golang.org/x/crypto v0.6.0
FATA[0000] 
Thanks for your PR.
For a PR to be accepted to Contour, it must have:
- at least one release-note label set
- a file named changelogs/unreleased/PR#-author-category,
  where category matches the release-note/category label you apply.

Error: Missing changelog file at ./changelogs/unreleased/5043-yangyy93-major.md
	
Please see the "Commit message and PR guidelines" section of CONTRIBUTING.md,
or https://github.com/projectcontour/contour/blob/main/design/changelog.md for background. 
exit status 1

@sidharthramesh
Copy link

sidharthramesh commented Mar 13, 2023

OMG is this happening!? Contour is wonderful except for the tracing support, it’s got everything I need. Thank you for this.

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

@yangyy93 just starting to go through this, had a few initial comments.

We will also need to add documentation for this great new feature, as a new page under site/content/docs/main/config. Please see other pages for an idea of what we're looking for.

cmd/contour/serve.go Outdated Show resolved Hide resolved
cmd/contour/servecontext_test.go Outdated Show resolved Hide resolved
pkg/config/parameters.go Outdated Show resolved Hide resolved
internal/dag/dag.go Outdated Show resolved Hide resolved
internal/dag/dag.go Outdated Show resolved Hide resolved
cmd/contour/serve.go Outdated Show resolved Hide resolved
@skriss
Copy link
Member

skriss commented Mar 15, 2023

FYI here's a very basic config that folks can use for testing:

Install otel operator:

kubectl apply -f https://github.com/open-telemetry/opentelemetry-operator/releases/latest/download/opentelemetry-operator.yaml

Install an otel collector instance, with verbose logging exporter enabled:

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
  name: simplest
spec:
  config: |
    receivers:
      otlp:
        protocols:
          grpc:
          http:
    processors:

    exporters:
      logging:
        verbosity: detailed

    service:
      pipelines:
        traces:
          receivers: [otlp]
          processors: []
          exporters: [logging]

Define extension service:

apiVersion: projectcontour.io/v1alpha1
kind: ExtensionService
metadata:
  name: otel-collector
spec:
  protocol: h2c
  services:
    - name: simplest-collector
      port: 4317

Update Contour config (needs a Contour deployment restart afterwards):

apiVersion: v1
kind: ConfigMap
metadata:
  name: contour
  namespace: projectcontour
data:
  contour.yaml: |
    tracing:
      extensionService: default/otel-collector
      serviceName: some-service-name
      customTags:
      - tagName: custom-tag
        literal: foo
      - tagName: header-tag
        requestHeaderName: X-Custom-Header

Now you should be able to see traces in the logs of the otel collector, e.g.:
kubectl logs deploy/simplest-collector

@yangyy93, it would be nice to include something similar to the above in examples, which could also be referenced from the documentation.

@skriss
Copy link
Member

skriss commented Mar 21, 2023

@yangyy93 this will need a rebase to resolve merge conflicts with recent merges, thanks.

@yangyy93
Copy link
Member Author

@skriss Thanks for your review, I will modify the code and add documentation according to your comments

@yangyy93 yangyy93 force-pushed the tracing branch 2 times, most recently from 35713cf to bc97340 Compare March 27, 2023 10:01
Signed-off-by: yy <[email protected]>

add some unit test

Signed-off-by: yy <[email protected]>

git rebase

Signed-off-by: yy <[email protected]>

expose configuration for envoy's RateLimitedAsResourceExhausted (projectcontour#4971)

The Rate Limit filter in Envoy translates a 429 HTTP response code
to UNAVAILABLE as specified in the gRPC mapping document, but Google recommends
translating it to RESOURCE_EXHAUSTED
(see https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md)

This commit introduces a new setting to allow contour to forward the same parameter
introduced in envoyproxy/envoy#4879

The default value is disabled to retain the original behaviour of returning UNAVAILABLE,
as changing it would be a breaking change.

Closes projectcontour#4901.

Signed-off-by: Víctor Roldán Betancort <[email protected]>
Signed-off-by: yy <[email protected]>

rebase

Signed-off-by: yy <[email protected]>

update tracing config validate

Signed-off-by: yy <[email protected]>

make generate

Signed-off-by: yy <[email protected]>

add chengelog

Signed-off-by: yy <[email protected]>

update make general

Signed-off-by: yy <[email protected]>

goimport

Signed-off-by: yy <[email protected]>

update tracing

Signed-off-by: yy <[email protected]>

fix golint

Signed-off-by: yy <[email protected]>

update test

Signed-off-by: yy <[email protected]>

delete unused code

Signed-off-by: yy <[email protected]>

delete error file

Signed-off-by: yy <[email protected]>

update changelog

Signed-off-by: yy <[email protected]>

fix some mistake

Signed-off-by: yy <[email protected]>

feat: Add HTTP support for External Auth (projectcontour#4994)

Support globally configuring an external auth
server which is enabled by default for all vhosts,
both HTTP and HTTPS.

Closes projectcontour#4954.

Signed-off-by: claytonig <[email protected]>
Signed-off-by: yy <[email protected]>

refactor DAG and DAG consumers to support >2 Listeners (projectcontour#5128)

Updates projectcontour#4960.

Signed-off-by: Steve Kriss <[email protected]>
Signed-off-by: yy <[email protected]>

resolve conflict

Signed-off-by: yy <[email protected]>

fix

Signed-off-by: yy <[email protected]>
Signed-off-by: yy <[email protected]>
@pavelnikolov
Copy link

Is this PR ready to be merged?

apis/projectcontour/v1alpha1/contourconfig.go Outdated Show resolved Hide resolved
apis/projectcontour/v1alpha1/contourconfig.go Outdated Show resolved Hide resolved
apis/projectcontour/v1alpha1/contourconfig.go Show resolved Hide resolved
site/content/docs/main/config/tracing.md Outdated Show resolved Hide resolved
yangyy93 added 2 commits April 6, 2023 17:56
Signed-off-by: yy <[email protected]>
Signed-off-by: yy <[email protected]>
pkg/config/parameters.go Outdated Show resolved Hide resolved
@skriss
Copy link
Member

skriss commented Apr 6, 2023

Just doing a little more testing on the latest version of code, but I think this is looking pretty good, thanks for the work on it @yangyy93!

Signed-off-by: yy <[email protected]>
Signed-off-by: yy <[email protected]>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @yangyy93, testing all checks out and this LGTM! Will leave for one more maintainer to approve.

@skriss
Copy link
Member

skriss commented Apr 13, 2023

I'm going to go ahead and merge this; if there is any after-the-fact feedback we still have some time to address before the 1.25 release.

@skriss skriss merged commit c813560 into projectcontour:main Apr 13, 2023
@pavelnikolov
Copy link

If you release an rc or alpha release, I'd be happy to go ahead and test this with our apps and provide feedback pre v1.25

@skriss
Copy link
Member

skriss commented Apr 13, 2023

If you release an rc or alpha release, I'd be happy to go ahead and test this with our apps and provide feedback pre v1.25

That'd be fantastic @pavelnikolov - we will be releasing an RC within the next week or two

@sidharthramesh
Copy link

Yay!

@rajatvig
Copy link
Member

💯

padlar pushed a commit to padlar/contour that referenced this pull request Apr 20, 2023
Adds support for exporting trace data
to OpenTelemetry.

Closes projectcontour#399.

Signed-off-by: yy <[email protected]>
@RmStorm
Copy link

RmStorm commented Apr 27, 2023

Is this RC release coming up sometime soon? I'm looking forward to trying it!

@skriss
Copy link
Member

skriss commented Apr 27, 2023

Is this RC release coming up sometime soon? I'm looking forward to trying it!

@RmStorm yes; we're a little behind schedule due to KubeCon but I hope to get it out today or tomorrow. Keep an eye out!

@RmStorm
Copy link

RmStorm commented May 3, 2023

Thanks for the release! I have been trying to get it to work but so far, no dice. My extensionservice looks like this:

apiVersion: projectcontour.io/v1alpha1
kind: ExtensionService
metadata:
  name: otel-traces
  namespace: observability
spec:
  protocol: h2c
  services:
    - name: tobs-opentelemetry-collector
      port: 4317

And in my contourDeployment I have a runtimeSettings block like this:

    runtimeSettings:
      envoy:
        listener:
          useProxyProtocol: true
      tracing:
        extensionService:
          name: otel-traces
          namespace: observability

I'm pretty sure this is correct but the tracing block does not show up in Envoy. I port-forwarded to one of the envoy pods and ran this command:

curl -s http://localhost:9001/config_dump | jq '.configs[] | select(.["@type"] == "type.googleapis.com/envoy.admin.v3.ListenersConfigDump") | .dynamic_listeners[] | select(.name == "ingress_http") | .active_state.listener.filter_chains'
Click for somewhat verbose Envoy config output
[
  {
    "filters": [
      {
        "name": "envoy.filters.network.http_connection_manager",
        "typed_config": {
          "@type": "type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager",
          "stat_prefix": "ingress_http",
          "rds": {
            "config_source": {
              "api_config_source": {
                "api_type": "GRPC",
                "grpc_services": [
                  {
                    "envoy_grpc": {
                      "cluster_name": "contour",
                      "authority": "contour"
                    }
                  }
                ],
                "transport_api_version": "V3"
              },
              "resource_api_version": "V3"
            },
            "route_config_name": "ingress_http"
          },
          "http_filters": [
            {
              "name": "compressor",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.filters.http.compressor.v3.Compressor",
                "compressor_library": {
                  "name": "gzip",
                  "typed_config": {
                    "@type": "type.googleapis.com/envoy.extensions.compression.gzip.compressor.v3.Gzip"
                  }
                },
                "response_direction_config": {
                  "common_config": {
                    "content_type": [
                      "text/html",
                      "text/plain",
                      "text/css",
                      "application/javascript",
                      "application/x-javascript",
                      "text/javascript",
                      "text/x-javascript",
                      "text/ecmascript",
                      "text/js",
                      "text/jscript",
                      "text/x-js",
                      "application/ecmascript",
                      "application/x-json",
                      "application/xml",
                      "application/json",
                      "image/svg+xml",
                      "text/xml",
                      "application/xhtml+xml",
                      "application/grpc-web",
                      "application/grpc-web+proto",
                      "application/grpc-web+json",
                      "application/grpc-web+thrift",
                      "application/grpc-web-text",
                      "application/grpc-web-text+proto",
                      "application/grpc-web-text+thrift"
                    ]
                  }
                }
              }
            },
            {
              "name": "grpcweb",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.filters.http.grpc_web.v3.GrpcWeb"
              }
            },
            {
              "name": "grpc_stats",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.filters.http.grpc_stats.v3.FilterConfig",
                "emit_filter_state": true,
                "enable_upstream_stats": true
              }
            },
            {
              "name": "cors",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.filters.http.cors.v3.Cors"
              }
            },
            {
              "name": "local_ratelimit",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.filters.http.local_ratelimit.v3.LocalRateLimit",
                "stat_prefix": "http"
              }
            },
            {
              "name": "envoy.filters.http.lua",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.filters.http.lua.v3.Lua",
                "default_source_code": {
                  "inline_string": "-- Placeholder for per-Route or per-Cluster overrides."
                }
              }
            },
            {
              "name": "envoy.filters.http.rbac",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC"
              }
            },
            {
              "name": "router",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.filters.http.router.v3.Router"
              }
            }
          ],
          "http_protocol_options": {
            "accept_http_10": true,
            "allow_chunked_length": true
          },
          "access_log": [
            {
              "name": "envoy.access_loggers.file",
              "typed_config": {
                "@type": "type.googleapis.com/envoy.extensions.access_loggers.file.v3.FileAccessLog",
                "path": "/dev/stdout"
              }
            }
          ],
          "use_remote_address": true,
          "normalize_path": true,
          "preserve_external_request_id": true,
          "merge_slashes": true,
          "common_http_protocol_options": {},
          "strip_any_host_port": true
        }
      }
    ]
  }
]

As you can see, this outputs the http_connection_manager block but it does not have the tracing block.

Any pointers on how to debug this would be much appreciated!

@sunjayBhatia
Copy link
Member

@RmStorm Hey there, thanks for testing out the RC! Looks like you're using the Gateway Provisioner to create Contour instances? If so would be good to confirm you've got all the CRDs and versions of images correct as a sanity check

Also, if you're modifying the ContourDeployment pointed to by a GatewayClass ParametersRef after you've already created a Gateway, you won't see the Contour installation for that Gateway get changed (in case that is what is going on here) you'll have to create a fresh Gateway after the change (due to the fact that the GatewayClass parameters are supposed to be a "snapshot" of what config to use and changing it according to the API guidelines should not cause a re-reconcile of any generated resources, this is a rough edge in Gateway API).

@skriss
Copy link
Member

skriss commented May 3, 2023

+1 to Sunjay's notes above. I just ran through this and was able to get a tracing-enabled Contour deployed successfully via the provisioner. Make sure you're using something like the following for installing the provisioner - this will have the updated CRDs and correct image tag:

kubectl apply -f https://projectcontour.io/quickstart/v1.25.0-rc.1/contour-gateway-provisioner.yaml

If you are updating an existing install per Sunjay's note above, then I'd suggest manually editing the ContourConfiguration resource and adding the tracing configuration there, since the provisioner won't update that if it's already been provisioned. You'll need to restart the contour deployment after updating the ContourConfiguration as well.

@RmStorm
Copy link

RmStorm commented May 4, 2023

Thanks that is very helpful. Manually updating the contourConfig and then rolling over the contour pods got the tracing config into envoy. 🎉 Very cool.

Now I ran head first into the next problem. I'm getting the traces from Envoy in my trace backend but none of the underlying services are picking up on them. I have inspected some requests and it is because the traceparent header is not set.

I guess concretely my question is, can I get Envoy to set headers for tracepropagation or must I use x-request-id? and if x-request-id is the way to go, what is the common way of going about it.

edit: disregard the struck out part of my comment, I was not careful yesterday and got things mixed up. The traceparent header is definitely set and I have traces working. Thanks for the great job getting it out!

@sunjayBhatia
Copy link
Member

nice! glad its working @RmStorm, feel free to try to kick the tires and break things, would be a great help before our release next week 👍🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/major A major change that needs more than a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracing Configuration
7 participants