Skip to content

Remove hardcoded AlwaysSample from OpenTelemetry tracing configuration #9229

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2025
Merged

Remove hardcoded AlwaysSample from OpenTelemetry tracing configuration #9229

merged 1 commit into from
Jul 11, 2025

Conversation

marcus-crane
Copy link
Contributor

Changes proposed by this PR

This pull request is a follow on the discussion raised here: #9227

Concourse currently hardcodes an AlwaysSample sampler in the OpenTelemetry SDK setup which generally means that this will always hold true. Any attempts to customise the sample rate via environment variables will not work as Go treats explicit SDK configuration as having precedent over anything else.

This should not change any existing behaviour as the default setting for sampling is to use the AlwaysOn sampler but it should become possible to set other samples + custom sample rate via the OTEL_TRACES_SAMPLER environment variable.

  • Removed AlwaysSample clause

Notes to reviewer

It's worth nothing that I haven't tested this out to confirm that it behaviour as I would assume it does. I need to get Concourse set up so I can do some local experiments and so on but at a glance, it should do what it says on the tin if the OpenTelemetry documentation is to be trusted.

Happy to have this PR in pending until I properly set it up locally though.

Release Note

  • Remove explicit OpenTelemetry 100% sample rate in order to support sample rate customisation via the OTEL_TRACES_SAMPLER environment variable.

@marcus-crane
Remove hardcoded AlwaysSample from OpenTelemetry tracing configuration
8dd6d26 
Signed-off-by: Marcus Crane <marcus.crane@halter.co.nz>
@marcus-crane marcus-crane requested a review from a team as a code owner July 8, 2025 08:47
@taylorsilva
Copy link
Member

I haven't used this in a while, but we do have a Docker compose override file that you can use to locally test OTEL stuff: https://github.com/concourse/concourse/blob/master/hack/overrides/jaeger.yml

To use this override, run the following in the root of the concourse/concourse repo (assuming you're on a unix-based OS):

docker compose -f ./docker-compose.yml -f ./hack/overrides/jaeger.yml up -d --build

You can then log into concourse using the steps here: https://github.com/concourse/concourse/blob/master/CONTRIBUTING.md#running-concourse

The jaeger.yml override is quite stale, so I made the following image tag bumps:

diff --git a/hack/overrides/jaeger.yml b/hack/overrides/jaeger.yml
index b5ef2bead..8b6dee3ee 100644
--- a/hack/overrides/jaeger.yml
+++ b/hack/overrides/jaeger.yml
@@ -27,7 +27,7 @@ services:
       - --config=/etc/config/otel-collector-config.yml

   jaeger:
-    image: jaegertracing/all-in-one:1.14
+    image: jaegertracing/all-in-one:latest
     command:
       - --sampling.strategies-file=/etc/jaeger/sampling_strategies.json
       - --log-level=debug
@@ -48,7 +48,7 @@ services:
       - "9411:9411"

   elasticsearch:
-    image: elasticsearch:7.6.1
+    image: elasticsearch:7.17.28
     volumes:
       - elasticsearch-data:/usr/share/elasticsearch/data
     ports:
@@ -58,7 +58,7 @@ services:
       discovery.type: single-node

   kibana:
-    image: kibana:7.6.1
+    image: kibana:7.17.28
     environment:
       ELASTICSEARCH_HOSTS: http://elasticsearch:9200
     ports:

If your local tests work as expected, I'll happily merge this in.

@marcus-crane
Copy link
Contributor Author

Thanks for the tips!

@marcus-crane
Copy link
Contributor Author

Default - All 10 traces appear in Jaegar (full sampling)

CleanShot 2025-07-11 at 18 36 37@2x CleanShot 2025-07-11 at 18 36 30@2x

Sampled - 5 traces (out of 10 runs) appear in Jaegar

Configuration used:

services:
  web:
    environment:
      CONCOURSE_TRACING_SERVICE_NAME: atc
      CONCOURSE_TRACING_OTLP_ADDRESS: jaeger:4317
      OTEL_TRACES_SAMPLER: traceidratio
      OTEL_TRACES_SAMPLER_ARG: 0.5
  worker:
    environment:
      CONCOURSE_TRACING_SERVICE_NAME: worker
      CONCOURSE_TRACING_OTLP_ADDRESS: jaeger:4317
      OTEL_TRACES_SAMPLER: traceidratio
      OTEL_TRACES_SAMPLER_ARG: 0.5
CleanShot 2025-07-11 at 18 42 32@2x CleanShot 2025-07-11 at 18 41 53@2x

@marcus-crane
Copy link
Contributor Author

It took me a while to actually get Jaeger working (as the config is a bit out of date) so I'll open a second PR since there might be some discussion. Locally, I eventually stubbed out the OpenTelemetry collector and just had traces shipped directly to Jaeger v2

@taylorsilva
Copy link
Member

That would be great if you could PR and update to those files 🙏 I hope to use them more too for testing stuff.

@taylorsilva taylorsilva merged commit ce537e4 into concourse:master Jul 11, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Pull Requests Jul 11, 2025
@marcus-crane marcus-crane mentioned this pull request Jul 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taylorsilva taylorsilva taylorsilva approved these changes

Assignees
No one assigned
Labels
misc
Projects
Pull Requests
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marcus-crane @taylorsilva