Skip to content

docker-compose: add otel-collector by default, disable jaeger by default#848

Merged
bobheadxi merged 5 commits into
masterfrom
otel-default
Sep 1, 2022
Merged

docker-compose: add otel-collector by default, disable jaeger by default#848
bobheadxi merged 5 commits into
masterfrom
otel-default

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Aug 25, 2022

Copy link
Copy Markdown
Member

Removes Jaeger from the default docker-compose deployment, and replaces it with OpenTelemetry Collector configured to log data only.

With the jaeger/docker-compose.yaml overlay, a Jaeger instance can be deployed and otel-collector will be configured to send traces to the deployed Jaeger instance.

The pure-docker version of this change is in #849

Closes https://github.com/sourcegraph/sourcegraph/issues/40455

Checklist

Test plan

docker-compose \
    -f docker-compose/docker-compose.yaml \
    -f docker-compose/jaeger/docker-compose.yaml \
    -f docker-compose/dev/docker-compose.yaml up
{
    "observability.tracing": { "type": "opentelemetry" }
}

Screenshot 2022-08-25 at 2 40 23 PM

@sanderginn sanderginn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It all looks good to me.

Is this repository meant to serve as a public deployment method? If so, we might want to offer means to set a custom otel config.

@bobheadxi

Copy link
Copy Markdown
Member Author

Is this repository meant to serve as a public deployment method? If so, we might want to offer means to set a custom otel config.

Yes - I added a mounted-by-default template similar to your approach in sourcegraph/deploy-sourcegraph#4163: 0b2f378

@bobheadxi bobheadxi marked this pull request as ready for review August 26, 2022 17:42
@bobheadxi bobheadxi requested a review from a team August 26, 2022 17:42

@sanderginn sanderginn left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

environment:
- 'SAMPLING_STRATEGIES_FILE=/etc/jaeger/sampling_strategies.json'

# Configure collector to send traces to Jaeger

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Configure collector to send traces to Jaeger
# Configure collector to send traces to Jaeger
# By default listens on the following ports:
# - grpc 4317
# - http 4318

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is already indicated on docstring for the underlying service configuration in the base docker-compose.yaml file, so I think we should be fine to omit this here

@michaellzc

Copy link
Copy Markdown
Member

docker compose stuff looks good to me, but how about our pure docker distro? https://github.com/sourcegraph/deploy-sourcegraph-docker/tree/master/pure-docker

I know there has not been much clear signal how to better support pure-docker distro in the future, but I do think we should back-port these changes to pure-docker as well.

@bobheadxi

bobheadxi commented Aug 30, 2022

Copy link
Copy Markdown
Member Author

pure-docker changes are here: #849 with some slight variations (still in draft)

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.

deploy-sourcegraph-docker: make Jaeger optional, always deploy otel-collector

4 participants