-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix(backend): tempo cloud connection string #3515
Conversation
@@ -27,6 +27,10 @@ type HttpClient struct { | |||
|
|||
func NewHttpClient(name string, config *datastore.HttpClientConfig, callback HttpCallback) DataSource { | |||
endpoint, _ := urlx.Parse(config.Url) | |||
if endpoint.Port() == "443" { | |||
endpoint.Scheme = "https" | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a url like this:
tempo-us-central1.grafana.net:443
The parsing method returns
http://tempo-us-central1.grafana.net:443
Which breaks the getTraceById
logic.
server/tracedb/tempodb.go
Outdated
@@ -23,7 +23,7 @@ import ( | |||
) | |||
|
|||
func tempoDefaultPorts() []string { | |||
return []string{"9095", ""} | |||
return []string{"9095", "443", "1"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
443 and 1 (using https://tempo-us-central1.grafana.net) should be also valid ports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen port 1 being used. Is there any docs about this port?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason the url parser uses 1
as port when using something like https://tempo-us-central1.grafana.net/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like a bug in the URL parsing library. Do you know why we're using an URL parser other than the go's stdlib one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That makes sense. What I would do is handle the case of port "1" and replace it by a correct default port, for example:
if port == 1 {
if schema == "http" {
port = 80
}
if schema == "https" {
port = 43
}
}
This is awesome. When will this be ready for me to try it? |
This PR fixes a confusing way of dealing with the tempo cloud http connection, as pasting the given endpoint from the webapp uses
:443
instead ofhttps
Changes
:443
port tohttps
Fixes
Checklist