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

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

Merged
merged 5 commits into from Sep 1, 2022

Conversation

bobheadxi
Copy link
Member

@bobheadxi bobheadxi commented Aug 25, 2022

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 sourcegraph/sourcegraph#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

Copy link

@sanderginn sanderginn left a comment

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
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
Copy link

@sanderginn sanderginn left a comment

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
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
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
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
Copy link
Member Author

bobheadxi commented Aug 30, 2022

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
4 participants