feature: adding grafana tempo cloud support #1963

xoscar · 2023-02-09T15:05:40Z

This PR adds support for the HTTP client for Grafana Tempo Cloud
It decouples the tempo config from just supporting gRPC and adds HTTP capabilities.

Changes

Updates the Tempo config to support the HTTP client
Adds section for gRPC and HTTP
Implements generic HTTP and gRPC client for the BE
Fixes issue with the test connection not supporting URLs with paths

Fixes

[DataStores] Research Connectivity issues with Grafana Cloud Tempo #1938

Checklist

tested locally
added new dependencies
updated the docs
added a test

https://www.loom.com/share/f17a9708c64341ab93e853cd5940c760

xoscar · 2023-02-09T15:06:13Z

api/dataStores.yaml

@@ -29,6 +29,15 @@ components:
        createdAt:
          type: string
          format: date-time
+    BaseClient:


Base client supports both http and grpc settings

xoscar · 2023-02-09T15:06:26Z

examples/tracetest-k6/docker-compose.yml

@@ -149,3 +149,12 @@ services:
        condition: service_healthy
      queue:
        condition: service_healthy
+
+  tempo:


Adding example including tempo

xoscar · 2023-02-09T15:09:33Z

server/http/mappings/data_store.go

-	deepcopy.DeepCopy(in.Tempo.Tls, &dataStore.Values.Tempo.TLSSetting)
-	deepcopy.DeepCopy(in.Tempo.Tls.Settings, &dataStore.Values.Tempo.TLSSetting.TLSSetting)
+	deepcopy.DeepCopy(in.Tempo.Grpc.Tls, &dataStore.Values.Tempo.Grpc.TLSSetting)
+	deepcopy.DeepCopy(in.Tempo.Grpc.Tls.Settings, &dataStore.Values.Tempo.Grpc.TLSSetting.TLSSetting)


We need to fix this at some point as its getting out of hand

xoscar · 2023-02-09T15:10:32Z

server/tracedb/client/client.go

+type SupportedClients string
+
+var (
+	GRPC SupportedClients = "grpc"


Main facade to use the http and grpc clients

xoscar · 2023-02-09T15:10:48Z

server/tracedb/client/grpc.go

+	}
+
+	client.conn = conn
+	return nil


Generic grpc client

xoscar · 2023-02-09T15:10:54Z

server/tracedb/client/http.go

+	"crypto/x509"
+	"errors"
+	"fmt"
+	"io"


Generic http client

Is it worth adding a generic http client? Every api is different and I can't see a benefit in having a generic client for it. I think we would have to make it extremely configurable, and I don't think that's worth the effort.

This might be applicable to the gRPC client too.

@schoren @danielbdias I want your thoughts here

The thing about having it extremely configurable is that the complexity of the generic client will explode in a little time (for instance, having a lot of ifs and options), that's correct?

To tame that, I was thinking if we can follow something like the strategy design pattern in the future: we could have one client for each DataStore and the data store itself decides how to use communication strategy internally (HTTP and/or gRPC).

@mathnogueira the gRPC is pretty much the same thing we were using for both tempo and jaeger just moved to a different file.

For the HTTP one, what things do you expect to keep changing? we already support the basic stuff including TLS.
Another thing is, this implementation is similar to the HTTP trigger one.

@danielbdias the initial idea was to build something similar to the strategy pattern, but as this is a double layer (trace db + clients) I think a better way to describe it is as an Abstract Factory.

The situation here is that tracedbs are all similar until the getTraceById step, where each one has a different way of handling that. I can create a data_store interface so each of the clients has the same implementation, and a single callback type

I updated the code based on the comments, let me know what you think

xoscar · 2023-02-09T15:12:44Z

server/tracedb/connection/connection.go

+		return false, err
+	}
+
+	endpoint = strings.TrimPrefix(endpoint, "http://")


Fixes an issue with urls like https://tempo-us-central1.grafana.net/tempo

xoscar · 2023-02-09T15:16:06Z

server/tracedb/tempodb.go

+	Batches []*traces.HttpResourceSpans `json:"batches"`
+}
+
+func httpGetTraceByID(ctx context.Context, traceID string, client client.HttpClient) (model.Trace, error) {


Adding HTTP trace processor

xoscar · 2023-02-09T15:16:45Z

server/traces/otel_http_converter.go

@@ -0,0 +1,112 @@
+package traces


There is a mismatch between tempo's HTTP response and the actual proto, I had to create this to fix that

xoscar · 2023-02-09T15:17:20Z

web/src/components/Settings/DataStorePlugin/forms/BaseClient/BaseClient.tsx

+  const Component = FieldsFormMap[type];
+
+  return (
+    <>


Implements switching from HTTP to GRPC

xoscar · 2023-02-09T15:18:05Z

web/src/components/Settings/DataStorePlugin/forms/HttpClient/HttpClient.tsx

+  const baseName = ['dataStore', dataStoreType, 'http'];
+  const insecureValue = Form.useWatch([...baseName, 'tls', 'insecure'], form) ?? true;
+
+  return (


new HTTP client

xoscar · 2023-02-09T15:18:54Z

web/src/services/DataStores/GrpcClient.service.ts

@@ -1,9 +1,7 @@
-import {IGRPCClientSettings, SupportedDataStores, TDataStoreService, TRawGRPCClientSettings} from 'types/Config.types';
-import DataStore from 'models/DataStore.model';


GRPC and HTTP clients are now out of the factory part to be used directly from the BaseClient & JaegerService

danielbdias · 2023-02-09T17:50:54Z

examples/tracetest-k6/collector.config.yaml

+  otlp/2:
+    endpoint: tempo-us-central1.grafana.net:443
+    headers:
+      Authorization: Basic Mzc0OTIzOmV5SnJJam9pTmpFeU1UaGlZalJtTURZM05UUmxaRGt4WmpSaE5UWTNZbVF5WVRKalpEVXdORE0zWWpnMU9DSXNJbTRpT2lKMGNtRmpaWFJsYzNRaUxDSnBaQ0k2TnpnNE5URXdmUT09


Should we have this secret here?

danielbdias · 2023-02-09T18:09:44Z

server/tracedb/client/client.go

+	HTTP HttpCallback
+}
+
+type Client struct {


This naming is difficult to understand because we are referencing as client.Client, which feels like "this is a client for what"? The idea behind it is here: https://github.com/golang/go/wiki/CodeReviewComments#package-names

At first glance, it seems to me something like TraceDataSource. Invoking other golang devs here: @schoren @mathnogueira , it makes sense folks?

I like TraceDataSource or just DataSource I'll give it a go, thanks for pointing me to that documentation file ❤️

danielbdias · 2023-02-09T18:23:21Z

server/traces/otel_http_converter.go

+
+	if value["doubleValue"] != nil {
+		val := value["doubleValue"].(float64)
+		if val != 0.0 {


I'm not sure if this comparison will work correctly every time, because of the floating point craziness that we have (like 0.1 * 10 != 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 + 0.1 that we have in some languages like Python and JS).

To check if it is a double here we could do:

val, ok := value["doubleValue"].(float64) if ok { // do stuff of `if val != 0.0` }

…-with-grafana-cloud-tempo

xoscar · 2023-02-09T19:04:14Z

Hey @danielbdias thanks for the review my man, I just updated some of the things you mentioned!

mathnogueira · 2023-02-09T19:34:14Z

I think it's worth adding an example using the http jaeger poller and add it to our CI

jorgeepc

Great job 🔥

feature: adding grafana tempo cloud support

04a5e9f

xoscar linked an issue Feb 9, 2023 that may be closed by this pull request

[DataStores] Research Connectivity issues with Grafana Cloud Tempo #1938

Closed

xoscar self-assigned this Feb 9, 2023

xoscar added frontend backend labels Feb 9, 2023

xoscar commented Feb 9, 2023

View reviewed changes

server/tracedb/client/grpc.go Outdated

}

client.conn = conn

return nil

Copy link

Collaborator Author

xoscar Feb 9, 2023

Choose a reason for hiding this comment

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

Generic grpc client

xoscar commented Feb 9, 2023

View reviewed changes

xoscar added 2 commits February 9, 2023 09:35

fixing cli tests

a5cefb2

fixing small bugs

17a755e

xoscar marked this pull request as ready for review February 9, 2023 15:52

xoscar requested review from mathnogueira, jorgeepc, schoren and danielbdias February 9, 2023 15:52

danielbdias reviewed Feb 9, 2023

View reviewed changes

xoscar mentioned this pull request Feb 9, 2023

[docs] Update Tempo documentation to match new setup #1966

Closed

xoscar added 2 commits February 9, 2023 12:52

Merge branch 'main' into 1938-datastores-research-connectivity-issues…

7f253c0

…-with-grafana-cloud-tempo

addressing feedback comments

84713b2

addressing feedback comments

493e258

danielbdias approved these changes Feb 10, 2023

View reviewed changes

jorgeepc approved these changes Feb 13, 2023

View reviewed changes

xoscar merged commit e659b1d into main Feb 13, 2023

xoscar deleted the 1938-datastores-research-connectivity-issues-with-grafana-cloud-tempo branch February 13, 2023 15:07

kdhamric mentioned this pull request Feb 21, 2023

Test connection for Tempo shows a false error message #2021

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: adding grafana tempo cloud support #1963

feature: adding grafana tempo cloud support #1963

xoscar commented Feb 9, 2023 •

edited

xoscar Feb 9, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

mathnogueira Feb 9, 2023

danielbdias Feb 10, 2023 •

edited

xoscar Feb 10, 2023

xoscar Feb 10, 2023

xoscar Feb 10, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

xoscar Feb 9, 2023

danielbdias Feb 9, 2023

danielbdias Feb 9, 2023

xoscar Feb 9, 2023

danielbdias Feb 9, 2023

xoscar commented Feb 9, 2023

mathnogueira commented Feb 9, 2023

jorgeepc left a comment

		@@ -1,9 +1,7 @@
		import {IGRPCClientSettings, SupportedDataStores, TDataStoreService, TRawGRPCClientSettings} from 'types/Config.types';
		import DataStore from 'models/DataStore.model';

feature: adding grafana tempo cloud support #1963

feature: adding grafana tempo cloud support #1963

Conversation

xoscar commented Feb 9, 2023 • edited

Changes

Fixes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbdias Feb 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xoscar commented Feb 9, 2023

mathnogueira commented Feb 9, 2023

jorgeepc left a comment

Choose a reason for hiding this comment

xoscar commented Feb 9, 2023 •

edited

danielbdias Feb 10, 2023 •

edited