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

Create Jaeger OTEL agent component #2216

Merged
merged 7 commits into from
May 5, 2020

Conversation

pavolloffay
Copy link
Member

Resolves #2204

This PR adds a new component - Jaeger Agent based on OpenTelemetry.

Notable changes:

  • the image name is `jaeger-opentelemetry-agent
  • UDP endpoints were removed from Jaeger OTEL collector
  • flag --reporter.grpc.host-port is used to configure Jaeger exporter endpoint and also fetch endpoint for sampling strategies in Jaeger receiver.
  • collector and agent use the same heath check and metrics endpoints
  • agent's main is in the collector's directory we need to use the same go module and reuse parts of the codebase.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@pavolloffay pavolloffay requested a review from a team as a code owner May 4, 2020 15:45
@pavolloffay pavolloffay requested review from jpkrohling and removed request for jpkrohling May 4, 2020 15:45
@pavolloffay
Copy link
Member Author

@rubenvp8510 or/and @objectiser could you please review?

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #2216 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2216      +/-   ##
==========================================
- Coverage   96.17%   96.16%   -0.01%     
==========================================
  Files         219      219              
  Lines       10632    10632              
==========================================
- Hits        10225    10224       -1     
  Misses        352      352              
- Partials       55       56       +1     
Impacted Files Coverage Δ
cmd/query/app/server.go 91.78% <0.00%> (-2.74%) ⬇️
cmd/flags/admin.go 79.36% <0.00%> (+1.58%) ⬆️

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 e43a11d...af67fb1. Read the comment docs.

Makefile Outdated Show resolved Hide resolved
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
}
otelCfg = c
}
err := defaults.MergeConfigs(cfg, otelCfg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the merge be inside the above if block? Merge only required if OTel config file provided?

Copy link
Member Author

Choose a reason for hiding this comment

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

The function correctly handles merge if the second parameter is nil

cmd := svc.Command()
jconfig.AddFlags(v,
cmd,
jflags.AddConfigFileFlag,
Copy link
Contributor

Choose a reason for hiding this comment

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

In the collector main it also has collectorApp.AddFlags - should the equivalent agentApp.AddFlags be here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed if there is no flag we want to consume.

This issue #2182 will do some hygiene on flags. We can either expose all or just expose what is used by the new component. For less confusion I think we should expose only what is being used.

pipelines:
traces:
exporters: [jaeger]
# TODO remove after https://github.com/open-telemetry/opentelemetry-collector/pull/910
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to have been merged.

Copy link
Member Author

@pavolloffay pavolloffay May 5, 2020

Choose a reason for hiding this comment

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

I will update this in a follow-up PR as it requires changes in files not related to this PR.

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.

Create Jaeger agent OpenTelemetry distribution
3 participants