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

Add jaeger-v2 single binary based on OTEL collector #4766

Merged
merged 15 commits into from
Sep 27, 2023

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Sep 16, 2023

Which problem is this PR solving?

Description of the changes

  • Adds a new binary jaeger-v2 using OTEL Collector framework
  • Minimal amount of extensions is included, to mimic what jaeger-collector normally has
  • It will combine all previous functions of agent/collector/query in one binary, but controllable via config file
$ go run -tags=ui ./cmd/jaeger-v2 --config ./cmd/jaeger-v2/config.yaml

Roadmap

https://docs.google.com/document/d/1s4_6VgAS7qAVp6iEm5KYvpiGw3h2Ja5T5HpKo29iv00/edit

Design

  • the ingestion and storing of traces will be done via standard receivers/processors/exporters OTEL Collector components
  • the jaeger-query and UI are implemented as jaeger_query extension (already working in this PR)

Storage

In order to keep the flexibility of mixing & matching storage implementations, all backends can be configured via jaeger_storage extension (we may need to add jaeger_metrics_storage extension in the future). It might look like this:

jaeger_storage:
  memory:  # defines Factory
    memstore:
      max_traces: 100000
  cassandra:
    cassandra_primary:
      servers: [...]
      namespace: jaeger
    cassandra_archive:
      servers: [...]
      namespace: jaeger_archive

The jaeger_query extension then references specific storage factories by name:

  jaeger_query:
    trace_storage: memstore
    dependencies: something_else
    metrics_store: prometheus_store

It's not clear yet if jaeger_query extension should simply subsume jaeger_storage extension, because Query is the only one that needs this generic access to storage, while things like exporters or Kafka ingester (receiver) always deal with a single implementation (because OTEL Coll pipeline allows to connect them with each other, which is not possible with extensions).

Trade-offs

  • This not using OTEL Collector builder ocb. That means people won't be able to assemble a different version of the collector with other extensions.
  • We may want to support ocb in the future, as it makes it easier to write custom in-process exporters for custom storage. It will require converting all the components into their own modules.

Next steps

  • Get feedback from the community on the approach
  • Fully implement all-in-one by wiring receivers / exporters correctly

Open Questions

  • How can we implement all-in-one equivalent that can be run without any config file?
  • Do we want healthcheckextension to be included by default?
  • Investigate startup error 2023-09-23T19:55:46.661-0400 warn zapgrpc/zapgrpc.go:195 [core] [Channel #2 SubChannel #3] grpc: addrConn.createTransport failed to connect to {Addr: ":16685", ServerName: "localhost:16685", }. Err: connection error: desc = "transport: Error while dialing: dial tcp :16685: connect: connection refused" {"grpc_log": true}

@yurishkuro yurishkuro requested a review from a team as a code owner September 16, 2023 01:52
@pavolloffay
Copy link
Member

I am not sure what benefits this approach will give us from the functionality standpoint. Also the otel mode is significantly different and our existing ecosystem is not compatible with it (operator, helm).

Personally, I would prefer maintaining ecosystem that is closer to OTEL (collector builder) and have an opinionated OTELcol build with Jaeger components.

@yurishkuro
Copy link
Member Author

yurishkuro commented Sep 20, 2023

@pavolloffay yeah, I tend to agree. Especially the binary size increase seems unreasonable to me.

The alternative approach is to define another binary, but still with a hardcoded list of extensions, like in this PR, without using ocb builder.

Do you have some thoughts on why we would want to use ocb for assembling the binary? If we organize all the extensions similar to OTEL contrib, then someone in theory could still use ocb for a custom build of Jaeger, but it's not clear to me what benefits this approach provides for our official binary.

@yurishkuro yurishkuro changed the title [WIP] Add otel command to collector [WIP] Add jaeger-v2 single binary based on OTEL collector Sep 23, 2023
@codecov
Copy link

codecov bot commented Sep 24, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Files Coverage Δ
cmd/query/app/flags.go 100.00% <ø> (ø)
cmd/query/app/server.go 94.57% <100.00%> (ø)
plugin/storage/memory/factory.go 85.29% <0.00%> (-14.71%) ⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
albertteoh
albertteoh previously approved these changes Sep 26, 2023
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

👍🏼 Amazing! Keen to see how this all fits together with the SPM feature, would be awesome to not require running a separate OTEL collector for the setup.

// add-ons
storageexporter.NewFactory(), // generic exporter to Jaeger v1 spanstore.SpanWriter
kafkaexporter.NewFactory(),
// elasticsearch.NewFactory(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exclude the elasticsearch exporter? Is it because storageexporter should already cover the ability to write to elasticsearch using jaeger's native elasticsearch span writer?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, because there isn't one, it's just the code I copied from Pavol's repo where es exporter module was defined, but was not implemented.

Initially yes, the generic support for existing storage backends is done via storageexporter, which converts OTLP into Jaeger model and sends to SpanWriter one span at a time. As we build OTLP-native exporters people can migrate to those.

plugin/storage/memory/factory.go Outdated Show resolved Hide resolved
Co-authored-by: Albert <26584478+albertteoh@users.noreply.github.com>
Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@yurishkuro yurishkuro changed the title [WIP] Add jaeger-v2 single binary based on OTEL collector Add jaeger-v2 single binary based on OTEL collector Sep 27, 2023
@yurishkuro yurishkuro merged commit f7736ed into jaegertracing:main Sep 27, 2023
30 of 31 checks passed
@yurishkuro yurishkuro deleted the add-otel-collector branch September 27, 2023 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants