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

[CHANGE] tempo-query: switch from grpcPlugin to standalone #3840

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

frzifus
Copy link
Contributor

@frzifus frzifus commented Jul 3, 2024

What this PR does:
This PR replaces the tempo-query gRPC storage plugin due to the deprecation in Jaeger 1.58.0 with a gRPC standalone service.

Instead of jaeger+tempo storage plugin, only the jaeger remote storage proxy is now delivered. This can be deployed as a sidecar to jaeger to achieve the same behavior again.

Tempo-Query will serve on 0.0.0.0:7777 by default.

Connect Jaeger-Query to Tempo-Query

SPAN_STORAGE_TYPE=grpc  jaeger-query --grpc-storage.server=localhost:7777

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

How to run the proxy locally

  1. Create a tempo configuration (tempo.yaml)
server:
  http_listen_port: 3200
  grpc_listen_port: 9096

distributor:
  receivers:
    otlp:
      protocols:
        grpc:
          endpoint: "localhost:4417"
        http:
          endpoint: "localhost:4418"

storage:
  trace:
    backend: local
    wal:
      path: /tmp/tempo/wal
    local:
      path: /tmp/tempo/blocks
  1. Run the tempo instance
docker run -v ${PWD}/tempo.yaml:/tempo.yaml:z --network=host --rm -it docker.io/grafana/tempo:latest -config.file=/tempo.yaml
  1. Create a tempo-query config (tempo-query.yaml)
address: "0.0.0.0:7777"
backend: "localhost:3200"

tls_enabled: false

tenant_header_key: "X-Tenant-ID"
services_query_duration: "10s"
  1. Run tempo-query.
docker run -v ${PWD}/query.yaml:/query.yaml:z  --rm -it --network=host localhost/tempo-query -config /query.yaml
  1. Run Jaeger and point to tempo-query.
docker run --rm -it --network=host  jaegertracing/jaeger-query:1.60 --grpc-storage.server="localhost:7777" --span-storage.type=grpc

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

* [CHANGE] **BREAKING CHANGE** Remove `autocomplete_filtering_enabled` feature flag [#3729](https://github.com/grafana/tempo/pull/3729) (@mapno)
* [CHANGE] Bump opentelemetry-collector to 0.102.1 [#3784](https://github.com/grafana/tempo/pull/3784) (@debasishbsws)
* [CHANGE] **BREAKING CHANGE** Bump tempo-query dependency to Jaeger 1.58.1 and switch from grpcPlugin to standalone server [#3840](https://github.com/grafana/tempo/issues/3840) (@frzifus)
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this breaking change surfaced to users? What do they need to do?

Does the configuration of tempo-query change?

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 configuration should be the same. The main change is that Jaeger is no longer part of the resulting image.

Instead of configuring the plugin with Jaeger a user needs to configure Jaeger itself to use the tempo-query proxy as storage backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any caveats that should be added to the documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly that the image no longer serves the tempo connection as a grpc-plugin loaded by jaeger. Instead its a standalone service which by default listens on 0.0.0.0:7777.

The configuration should remain the same, aside from the new address field e.g.:

+ address: "0.0.0.0:7777"
backend: "my-backend-service"

tls_enabled: true
tls:
  cert_path: "/path/to/server.crt"
  key_path: "/path/to/server.key"
  ca_path: "/path/to/ca.crt"
  server_name: "my.server.name"
  insecure_skip_verify: false
  cipher_suites: "TTLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256"
  min_version: "TLS1.2"

tenant_header_key: "X-Tenant-ID"
services_query_duration: "10s"

cmd/tempo-query/Dockerfile Show resolved Hide resolved
@frzifus frzifus force-pushed the bump/tempo-query branch 3 times, most recently from 157091e to 6b3889e Compare July 4, 2024 14:54
@frzifus frzifus force-pushed the bump/tempo-query branch 2 times, most recently from a9d19fc to b7a23f6 Compare August 13, 2024 15:54
@frzifus frzifus marked this pull request as ready for review August 13, 2024 15:55
@frzifus frzifus requested a review from pavolloffay August 13, 2024 16:27
@frzifus
Copy link
Contributor Author

frzifus commented Aug 13, 2024

I think we need to explicitly forward the tenent header and that commit is missing. I will double check and update this later today or tomorrow morning.

@frzifus frzifus marked this pull request as draft August 15, 2024 09:08
@frzifus
Copy link
Contributor Author

frzifus commented Aug 15, 2024

Changed the PR back to draft mode. Ive to adapt integration/e2e/query_plugin_test.go.

@frzifus
Copy link
Contributor Author

frzifus commented Aug 29, 2024

mh.. seems instead of using the new tempo-query impl. the integration test still uses the modified jaeger image? :X

log
17:55:23 Starting tempo-query
17:55:23 tempo-query: 2024/08/28 15:55:23 maxprocs: Leaving GOMAXPROCS=8: CPU quota undefined
17:55:23 tempo-query: 2024/08/28 15:55:23 application version: git-commit=55e991a29725468164b11be5fc4e260dc09598d6, git-version=v1.57.0, build-date=2024-05-01T23:19:12Z
17:55:23 tempo-query: Error: unknown shorthand flag: 'c' in -config=/shared/config-tempo-query.yaml
17:55:23 tempo-query: unknown shorthand flag: 'c' in -config=/shared/config-tempo-query.yaml
17:55:23 tempo-query: Usage:
17:55:23 tempo-query: jaeger-query [flags]
17:55:23 tempo-query: jaeger-query [command]
17:55:23 tempo-query: Available Commands:
17:55:23 tempo-query: completion   Generate the autocompletion script for the specified shell
17:55:23 tempo-query: docs         Generates documentation
17:55:23 tempo-query: env          Help about environment variables.
17:55:23 tempo-query: help         Help about any command
17:55:23 tempo-query: print-config Print names and values of configuration options
17:55:23 tempo-query: status       Print the status.
17:55:23 tempo-query: version      Print the version.
17:55:23 tempo-query: Flags:
17:55:23 tempo-query: --admin.http.host-port string                     The host:port (e.g. 127.0.0.1:16687 or :16687) for the admin server, including health check, /metrics, etc. (default ":16687")
17:55:23 tempo-query: --admin.http.tls.cert string                      Path to a TLS Certificate file, used to identify this server to clients
17:55:23 tempo-query: --admin.http.tls.cipher-suites string             Comma-separated list of cipher suites for the server, values are from tls package constants (https://golang.org/pkg/crypto/tls/#pkg-constants).
17:55:23 tempo-query: --admin.http.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)
17:55:23 tempo-query: --admin.http.tls.enabled                          Enable TLS on the server
17:55:23 tempo-query: --admin.http.tls.key string                       Path to a TLS Private Key file, used to identify this server to clients
17:55:23 tempo-query: --admin.http.tls.max-version string               Maximum TLS version supported (Possible values: 1.0, 1.1, 1.2, 1.3)
17:55:23 tempo-query: --admin.http.tls.min-version string               Minimum TLS version supported (Possible values: 1.0, 1.1, 1.2, 1.3)
17:55:23 tempo-query: --config-file string                              Configuration file in JSON, TOML, YAML, HCL, or Java properties formats (default none). See spf13/viper for precedence.
17:55:23 tempo-query: --grpc-storage-plugin.binary string               (deprecated, will be removed after 2024-03-01) The location of the plugin binary
17:55:23 tempo-query: --grpc-storage-plugin.configuration-file string   (deprecated, will be removed after 2024-03-01) A path pointing to the plugin's configuration file, made available to the plugin with the --config arg
17:55:23 tempo-query: --grpc-storage-plugin.log-level string            Set the log level of the plugin's logger (default "warn")
17:55:23 tempo-query: --grpc-storage.connection-timeout duration        The remote storage gRPC server connection timeout (default 5s)
17:55:23 tempo-query: --grpc-storage.server string                      The remote storage gRPC server address as host:port
17:55:23 tempo-query: --grpc-storage.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)
17:55:23 tempo-query: --grpc-storage.tls.cert string                    Path to a TLS Certificate file, used to identify this process to the remote server(s)
17:55:23 tempo-query: --grpc-storage.tls.enabled                        Enable TLS when talking to the remote server(s)
17:55:23 tempo-query: --grpc-storage.tls.key string                     Path to a TLS Private Key file, used to identify this process to the remote server(s)
17:55:23 tempo-query: --grpc-storage.tls.server-name string             Override the TLS server name we expect in the certificate of the remote server(s)
17:55:23 tempo-query: --grpc-storage.tls.skip-host-verify               (insecure) Skip server's certificate chain and host name verification
17:55:23 tempo-query: -h, --help                                            help for jaeger-query
17:55:23 tempo-query: --log-level string                                Minimal allowed log Level. For more levels see https://github.com/uber-go/zap (default "info")
17:55:23 tempo-query: --metrics-backend string                          Defines which metrics backend to use for metrics reporting: prometheus, none, or expvar (deprecated, will be removed after 2024-01-01 or in release v1.53.0, whichever is later)  (default "prometheus")
17:55:23 tempo-query: --metrics-http-route string                       Defines the route of HTTP endpoint for metrics backends that support scraping (default "/metrics")
17:55:23 tempo-query: --multi-tenancy.enabled                           Enable tenancy header when receiving or querying
17:55:23 tempo-query: --multi-tenancy.header string                     HTTP header carrying tenant (default "x-tenant")

@frzifus
Copy link
Contributor Author

frzifus commented Aug 29, 2024

@frzifus frzifus marked this pull request as ready for review August 29, 2024 09:58
Jaeger 1.58 no longer supports gRPC storage plugins.

Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
@joe-elliott
Copy link
Member

I apologize for letting this sit, but I don't know much about this. Is this removing the Jaeger UI? I believe RedHat/OpenShift rely on this, but I honestly don't know.

@pavolloffay @andreasgerstmayr
if you are good on these changes I will merge. RH is the primary user of tempo-query.

@frzifus
Copy link
Contributor Author

frzifus commented Aug 29, 2024

I apologize for letting this sit, but I don't know much about this. Is this removing the Jaeger UI? I believe RedHat/OpenShift rely on this, but I honestly don't know.

@pavolloffay @andreasgerstmayr if you are good on these changes I will merge. RH is the primary user of tempo-query.

Maybe I should also have provided more context. ^^

Up to and including Jaeger 1.58, tempo-query was a Jaeger instance with an installed custom plugin for connecting to tempo. The plugin option was removed in Jaeger 1.59.

But the remote grpc storage is still supported.
By changing the plugin into a standalone proxy, we can deploy it as a sidecar and bump the jaeger version independently.

I am currently working on a patch for the tempo operator.

@joe-elliott joe-elliott merged commit 2999520 into grafana:main Sep 3, 2024
16 checks passed
@frzifus frzifus deleted the bump/tempo-query branch September 3, 2024 17:14
pavolloffay pushed a commit to pavolloffay/tempo that referenced this pull request Oct 10, 2024
)

* tempo-query: switch from grpcPlugin to standalone

Jaeger 1.58 no longer supports gRPC storage plugins.

Signed-off-by: Benedikt Bongartz <[email protected]>

* add changelog entry

Signed-off-by: Benedikt Bongartz <[email protected]>

* update vendor

Signed-off-by: Benedikt Bongartz <[email protected]>

* fix: panic when requesting dependencies

Signed-off-by: Benedikt Bongartz <[email protected]>

* integration/e2e: adapt tempo-query test

Signed-off-by: Benedikt Bongartz <[email protected]>

* changelog: update

Signed-off-by: Benedikt Bongartz <[email protected]>

---------

Signed-off-by: Benedikt Bongartz <[email protected]>
joe-elliott pushed a commit that referenced this pull request Oct 10, 2024
* [CHANGE] tempo-query: switch from grpcPlugin to standalone (#3840)

* tempo-query: switch from grpcPlugin to standalone

Jaeger 1.58 no longer supports gRPC storage plugins.

Signed-off-by: Benedikt Bongartz <[email protected]>

* add changelog entry

Signed-off-by: Benedikt Bongartz <[email protected]>

* update vendor

Signed-off-by: Benedikt Bongartz <[email protected]>

* fix: panic when requesting dependencies

Signed-off-by: Benedikt Bongartz <[email protected]>

* integration/e2e: adapt tempo-query test

Signed-off-by: Benedikt Bongartz <[email protected]>

* changelog: update

Signed-off-by: Benedikt Bongartz <[email protected]>

---------

Signed-off-by: Benedikt Bongartz <[email protected]>

* Backport tempo-query: concurrent trace search and opentelemetry

Signed-off-by: Pavol Loffay <[email protected]>

* Fix

Signed-off-by: Pavol Loffay <[email protected]>

---------

Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Co-authored-by: Ben B. <[email protected]>
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.

5 participants