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

Update k8s config to use opentelemetry docker image and configuration #459

Merged
merged 4 commits into from
Jan 13, 2020

Conversation

ccaraman
Copy link
Contributor

Resolves #366 by updating configuration to use Otel model and v0.2.0 docker image.

The docker compose instructions and configs are correct and up to date https://github.com/open-telemetry/opentelemetry-collector/tree/master/examples/demo

@codecov-io
Copy link

codecov-io commented Dec 11, 2019

Codecov Report

Merging #459 into master will increase coverage by 1.56%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
+ Coverage   75.72%   77.28%   +1.56%     
==========================================
  Files         120      121       +1     
  Lines        7345     8528    +1183     
==========================================
+ Hits         5562     6591    +1029     
- Misses       1520     1652     +132     
- Partials      263      285      +22
Impacted Files Coverage Δ
extension/pprofextension/pprofextension.go 82.35% <0%> (-6.54%) ⬇️
processor/spanprocessor/span.go 86.88% <0%> (-3.12%) ⬇️
processor/resourceprocessor/resource_processor.go 89.87% <0%> (-2.44%) ⬇️
processor/attributesprocessor/attributes.go 94.53% <0%> (-0.88%) ⬇️
exporter/exportertest/sink_exporter.go 72.72% <0%> (-0.61%) ⬇️
exporter/exporterhelper/tracehelper.go 94.91% <0%> (-0.54%) ⬇️
exporter/exporterhelper/metricshelper.go 95.45% <0%> (-0.47%) ⬇️
receiver/jaegerreceiver/factory.go 91.81% <0%> (-0.32%) ⬇️
receiver/prometheusreceiver/metrics_receiver.go 74.19% <0%> (-0.28%) ⬇️
processor/processortest/nop_processor.go 81.57% <0%> (-0.24%) ⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94066e4...9d46f3a. Read the comment docs.

# port: 9411
- name: jaeger-tchannel
port: 14267
- name: jaeger_thrift_http
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for consistency let's use either - or _

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll use lower case for consistency - but it doesn't matter too much for this as they are just the names for the ports.

num_workers: 8
reconnection_delay: 2s
secure: true
zipkin: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we list down options for other core exporters/receivers in the examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal of this config is to give everyone a starting point for k8s - the config.yaml for each component(exporter, receiver, etc) give more detailed examples.

reconnection_delay: 2s
secure: true
zipkin: {}
jaeger: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

wasn't jaeger split into the jaeger_grpc and jaeger_thrift_http exporters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - didn't realise that happened in the v0.2.0 release updating that now

- "--config=/conf/otel-agent-config.yaml"
image: omnition/opencensus-agent:0.1.6
image: omnition/opentelemetry-collectort:v0.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

the image name has a typo with an extra t at the end and should be:

image: omnition/opentelemetry-collector:v0.2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - i'll update :)

@ccaraman
Copy link
Contributor Author

@owais and @secat ready for another pass :)

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Thanks @ccaraman LGTM, two minor suggestions.

examples/k8s.yaml Show resolved Hide resolved
examples/k8s.yaml Show resolved Hide resolved
# - containerPort: 14267
# - containerPort: 14268
# - containerPort: 9411
- containerPort: 55678 # Default Opencensus receiver port.
Copy link
Member

Choose a reason for hiding this comment

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

In the current ConfigMap for otel-agnet, the open census endpoint is not set

I am not sure if it means the open census receiver is open or not, while the container port 55678 is open here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OpenCensus receiver has a default port so once it is specified in the receiver list it will be accepting traffic on 55678.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM

@pjanotti pjanotti merged commit 4e01fa3 into master Jan 13, 2020
@pjanotti pjanotti deleted the feature/ccaraman/update_examples branch January 13, 2020 20:26
subnova pushed a commit to subnova/opentelemetry-collector that referenced this pull request Jan 14, 2020
…open-telemetry#459)

* Update k8s config to use opentelemetry docker image and configuration

* Address pr comments

* Add queued retry to agent config

* Set memory ballast cli parameter
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
…etry#459)

* add simplified export pipeline setup for Jaeger

* add With* options to configure SDK options.

* add test for WithRegistration and WithSDK

* rename Registeration with RegisterGlobal

* rename WithRegistration to RegisterAsGlobal

Co-authored-by: rahulpa <rahulpa@google.com>
Co-authored-by: Joshua MacDonald <jmacd@users.noreply.github.com>
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
…y#459)

Bumps [boto3](https://github.com/boto/boto3) from 1.17.89 to 1.17.90.
- [Release notes](https://github.com/boto/boto3/releases)
- [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst)
- [Commits](boto/boto3@1.17.89...1.17.90)

---
updated-dependencies:
- dependency-name: boto3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

Update examples to use OpenTelemetry configuration
6 participants