-
Notifications
You must be signed in to change notification settings - Fork 196
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
Update qryn+gigapipe docs #1594
Conversation
@blumamir we should be nearly ready! |
@blumamir the blocker seems to be related to secrets. Could you review our changes and suggest what's wrong? |
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.
Thanks @lmangani
This looks great, thank you for improving it so nicely and making sure everything works.
I've added few comments, most of them are nits or suggestions, so feel free to address what you like and ignore what you don't want to do.
- **API Username**: The HTTP Basic Auth username for your qryn instance | ||
- **API Password**: The HTTP Basic Auth password for your qryn instance |
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.
If I got it right, those are the value in the QRYN_LOGIN
and QRYN_PASSWORD
environment variables described in the docs?
WDYT about making it explicit, so users know where they can find the content of these values to populate here quickly.
docs/backends/qryn.mdx
Outdated
- **Name**: Choose a name for this backend _(e.g. qryn)_ | ||
- **API Username**: The HTTP Basic Auth username for your qryn instance | ||
- **API Password**: The HTTP Basic Auth password for your qryn instance | ||
- **API URL**: The API Endpoint for for your qryn instance _(e.g. https://qryn.local:3100)_ |
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.
The example here shows https
. I wonder is http
supported as well? and if so, which one is more common?
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 the OSS version http is the default indeed. we can set the example to plain http
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.
amazing thanks, I think it can avoid confusion for some users 🚀
- **API Secret**: The API Secret for your Gigapipe Account | ||
- **API Key**: The API Token for your Gigapipe Account |
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.
We experienced that users sometimes struggle to find out where in the managed backend ui console these values can be found.
If you find it helpful, you can browse other destinations and consider adding some instructions or screenshot on how to populate these values. for example
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.
We included a Frame with a screenshot but we'll sure update it with a more specific one as we proceed.
return fmt.Sprintf("%s://%s:%s@%s", u.Scheme, apiKey, apiSecret, u.Host), nil | ||
} | ||
|
||
func (g *Qryn) maybeAddExporterName(conf *qrynConf, currentConfig *Config, processorName string, name string, |
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.
Can you elaborate on the usage for this exporter name?
I see it adds an attribute "qryn_exporter"
to all telemetry signals with values odigos-qryn-logs
, odigos-qryn-traces
, odigos-qryn-metrics
.
Is this added to trace in qryn where the data came from?
Is qryn_exporter
a common attribute in qryn
ecosystem?
common/config/qryn.go
Outdated
@@ -101,5 +149,22 @@ func parseURL(rawURL, apiKey, apiSecret string) (string, error) { | |||
return parseURL(fmt.Sprintf("https://%s", rawURL), apiKey, apiSecret) | |||
} | |||
|
|||
return fmt.Sprintf("https://%s:%s@%s", apiKey, apiSecret, u.Host), nil | |||
return fmt.Sprintf("%s://%s:%s@%s", u.Scheme, apiKey, apiSecret, u.Host), nil |
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.
important: if we encode the secret into the url this way, it will show up as plain text in the config map which can be undesirable for some users due to security concerns.
You can use the basicauthextension
to implement this authentication, while keeping the password as env variable. see otlphttp
for example code on how to use it
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.
Update: after the latest commit, the value is no longer plain text in the configmap which is great.
If you like, you can still consider using the basic auto extension, or resolve this thread.
common/config/qryn.go
Outdated
@@ -101,5 +149,22 @@ func parseURL(rawURL, apiKey, apiSecret string) (string, error) { | |||
return parseURL(fmt.Sprintf("https://%s", rawURL), apiKey, apiSecret) | |||
} | |||
|
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.
Consider trimming rawURL white spaces, as it will make url.Parse(rawURL)
return with error.
noWhiteSpaces := strings.TrimSpace(rawUrl)
parsedUrl, err := url.Parse(noWhiteSpaces)
common/config/qryn.go
Outdated
@@ -101,5 +149,22 @@ func parseURL(rawURL, apiKey, apiSecret string) (string, error) { | |||
return parseURL(fmt.Sprintf("https://%s", rawURL), apiKey, apiSecret) | |||
} | |||
|
|||
return fmt.Sprintf("https://%s:%s@%s", apiKey, apiSecret, u.Host), nil | |||
return fmt.Sprintf("%s://%s:%s@%s", u.Scheme, apiKey, apiSecret, u.Host), nil |
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.
if we just take the u.Host
, we will silently drop any u.Path
that the user might have set. I guess in most cases this is fine, but in clusters with internal routing this might break the export.
I am also fine with keeping it as is if you prefer and address this issue if someone bumps into it.
An alternative can be to modify the u
object itself, and then use u.String()
to make it a safe url string in idomatic way
Co-authored-by: Amir Blum <[email protected]>
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.
Thanks again qryn team, you are awesome.
Added few more minor comments, but they are all optional.
Feel free to adddress what makes sense to you and resolve the rest and then we can merge.
destinations/data/qrynoss.yaml
Outdated
displayName: Basic auth password | ||
componentType: input | ||
componentProps: | ||
type: password |
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.
you can consider adding:
tooltip: 'some short text describing this option'
It will then show up in the UI as a question mark popup next to the option, giving more context on how to fecth it or what it does.
docs/backends/qryn.mdx
Outdated
- **Name**: Choose a name for this backend _(e.g. qryn)_ | ||
- **API Username**: The HTTP Basic Auth username for your qryn instance | ||
- **API Password**: The HTTP Basic Auth password for your qryn instance | ||
- **API URL**: The API Endpoint for for your qryn instance _(e.g. https://qryn.local:3100)_ |
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.
amazing thanks, I think it can avoid confusion for some users 🚀
common/config/qryn.go
Outdated
@@ -101,5 +149,22 @@ func parseURL(rawURL, apiKey, apiSecret string) (string, error) { | |||
return parseURL(fmt.Sprintf("https://%s", rawURL), apiKey, apiSecret) | |||
} | |||
|
|||
return fmt.Sprintf("https://%s:%s@%s", apiKey, apiSecret, u.Host), nil | |||
return fmt.Sprintf("%s://%s:%s@%s", u.Scheme, apiKey, apiSecret, u.Host), nil |
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.
Update: after the latest commit, the value is no longer plain text in the configmap which is great.
If you like, you can still consider using the basic auto extension, or resolve this thread.
@blumamir I think we're ready! Please ping us on the side to provide you some testing tokens for validation 😉 |
Thanks, merged the PR. |
Updates for the qryn opensource and qryn/gigapipe integrations