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

Introduce new flags for Jaeger OTEL components #2246

Closed
pavolloffay opened this issue May 15, 2020 · 12 comments
Closed

Introduce new flags for Jaeger OTEL components #2246

pavolloffay opened this issue May 15, 2020 · 12 comments

Comments

@pavolloffay
Copy link
Member

At the moment Jaeger OTEL collector exposes legacy Jaeger flags. This was a priority to make migration as easy as possible (almost drop-in replacement). However in the context of the OTEL collector these flags are hard to understand because they are not properly scoped under appropriate OTEL components (receiver, exporter etc.).

Therefore I propose to keep legacy flags as deprecated and introduce a set of new flags that will be better aligned with OTEL components. For instance, Jaeger receiver flags could have prefix --recevier.jaeger and exporter --exporter.jaeger. We can discuss other naming conventions.

Current set of flags exposed by Jaeger OTEL components

Jaeger exporter flags and receiver for sampling strategies(to retrieve sampling strategies from remote collector):

      --reporter.grpc.host-port string                     Comma-separated string representing host:port of a static list of collectors to connect to directly
      --reporter.grpc.tls                                  (deprecated) see --reporter.grpc.tls.enabled
      --reporter.grpc.tls.ca string                        Path to a TLS CA (Certification Authority) file used to verify the remote server(s) (by default will use the system truststore)
      --reporter.grpc.tls.cert string                      Path to a TLS Certificate file, used to identify this process to the remote server(s)
      --reporter.grpc.tls.enabled                          Enable TLS when talking to the remote server(s)
      --reporter.grpc.tls.key string                       Path to a TLS Private Key file, used to identify this process to the remote server(s)
      --reporter.grpc.tls.server-name string               Override the TLS server name we expect in the certificate of the remove server(s)
      --reporter.grpc.tls.skip-host-verify                 (insecure) Skip server's certificate chain and host name verification

Jaeger receiver flags:

      --processor.jaeger-binary.server-host-port string    host:port for the UDP server (default ":6832")
      --processor.jaeger-compact.server-host-port string   host:port for the UDP server (default ":6831")

      --collector.grpc-server.host-port string             The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's GRPC server (default ":14250")
      --collector.grpc.tls                                 (deprecated) see --collector.grpc.tls.enabled
      --collector.grpc.tls.cert string                     Path to a TLS Certificate file, used to identify this server to clients
      --collector.grpc.tls.client-ca string                Path to a TLS CA (Certification Authority) file used to verify certificates presented by clients (if unset, all clients are permitted)
      --collector.grpc.tls.client.ca string                (deprecated) see --collector.grpc.tls.client-ca
      --collector.grpc.tls.enabled                         Enable TLS on the server
      --collector.grpc.tls.key string                      Path to a TLS Private Key file, used to identify this server to clients

      --collector.http-server.host-port string             The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's HTTP server (default ":14268")
      --collector.zipkin.host-port string                  The host:port (e.g. 127.0.0.1:5555 or :5555) of the collector's Zipkin server (default ":0")
      --http-server.host-port string                       host:port of the http server (e.g. for /sampling point and /baggageRestrictions endpoint) (default ":5778")

      --jaeger.tags string                                 (deprecated, use --resource.labels) One or more tags to be added to the Process tags of all spans passing through this agent. Ex: key1=value1,key2=${envVar:defaultValue}

      --sampling.strategies-file string                    The path for the sampling strategies file in JSON format. See sampling documentation to see format of the file

Attribute processor tags:

      --jaeger.tags string                                 (deprecated, use --resource.labels) One or more tags to be added to the Process tags of all spans passing through this agent. Ex: key1=value1,key2=${envVar:defaultValue}

Other flags:

      --config-file string                                 Configuration file in JSON, TOML, YAML, HCL, or Java properties formats (default none). See spf13/viper for precedence.

cc) @jaegertracing/jaeger-maintainers

@pavolloffay
Copy link
Member Author

The deprecated flags would be removed later in the future e.g. 2.2 release.

@objectiser
Copy link
Contributor

Agree with the approach.

Although, as migration from existing Jaeger components to the OTelCollector based components may not happen immediately, we probably will need to leave them in for longer than a couple of minor releases.

Would it also be possible to have use of deprecated flags reported as a warning? To act as a reminder to update.

@pavolloffay
Copy link
Member Author

Yes, that would be ideal. Viper exposes IsSet that can be used for this purpose.

@yurishkuro
Copy link
Member

Since the new flags won't satisfy the drop-in replacement requirement, do we even need them? As I understand the real OTEL collector doesn't support flags, only config file. Could we just go with their approach as well?

@pavolloffay
Copy link
Member Author

Could we just go with their approach as well?

This might be controversial since we provide the default configuration the flags make it easy to run without the config file. For instance in agent use case just specify the reporter's endpoint.

If we eventually remove all flags and still use the default configuration users will have to provide the config file but just fill in the important parts and not the full pipeline e.g.:

exporters:
  jaeger:
    endpoint: jaeger:14250

@yurishkuro
Copy link
Member

I think OTEL also provides the notion of default configuration. So it's really the question of overriding just very small portions, like a single address.

I don't have a strong opinion. It seems that config-only approach is pretty common, e.g. Prometheus also uses it afaik, not just OTC. But then both OTC and Prometheus are just single binaries, whereas Jaeger components need to be connected together, which is harder with configs (e.g. how to bring the full stack in docker-compose?)

@pavolloffay
Copy link
Member Author

I think OTEL also provides the notion of default configuration.

Could you provide more details? I don't think there is a default configuration in the sense that OTEL will use a set of components if the config is not provided.

@yurishkuro
Copy link
Member

Basing it purely on remembering seeing functions like "getDefaultConfig". You may be right that the selection of components is not defaulted.

@jpkrohling
Copy link
Contributor

almost drop-in replacement

What is preventing it from being a drop-in replacement?

@pavolloffay
Copy link
Member Author

@jpkrohling
Copy link
Contributor

The differences are:

Health check port changed to 13133
Not all current Jaeger flags are exposed (e.g. health check port)
Exposed metrics

This? If so, then the flags people are currently using should just work with the new components (except for health check port), right? That's pretty minor, IMO.

@pavolloffay
Copy link
Member Author

Correct, although it's worth documenting all the changes and then compile that to a migration guide. Also not all flags are supported in the OTEL build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants