-
Notifications
You must be signed in to change notification settings - Fork 487
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
Add grafanacloud-specific install script #291
Conversation
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.
lgtm for gcloud, 👍 /👎 works with your stack?
Co-authored-by: Evan <10603766+hoenn@users.noreply.github.com>
yeah, it generated a sane config for my stack |
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.
Bash script looks good to me, one change requested to fix the required variable messages but approved to avoid blocking :)
One other nit is that there is some trailing whitespace throughout the script which has been highlighted by my editor.
@@ -0,0 +1,80 @@ | |||
// Package grafanacloud provides an interface to the Grafana Cloud API. | |||
package grafanacloud |
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.
This is out of scope but we really should start consolidating our GCom clients 🙂
// http client will be used instead. | ||
// | ||
// apiKey will be used to authenticate against the API. | ||
func NewClient(c *http.Client, apiKey string) *Client { |
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.
What do you think about adding NewClientFromEnv
that assumes a configured GCLOUD_API_KEY
?
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 think the client code is too low level for environment variable logic. The caller can be responsible for retrieving environment variables, if necessary.
(That being said, I don't really like environment variables for configuration, and I think having the flag here is enough given this is a hidden command we don't want to document)
func (c *Client) AgentConfig(ctx context.Context, stackID string) (string, error) { | ||
req, err := http.NewRequestWithContext( | ||
ctx, "GET", | ||
fmt.Sprintf("%s/stacks/%s/agent_config", integrationsAPIURL, stackID), |
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.
What do you think about using %s/stacks/%s/agent_config.yaml
instead? What logic should live in the client?
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.
Does that endpoint exist? I get a 404 when I try it.
Given the Agent will be responsible for stitching together pieces of information in the future, I think it's fine for the logic to exist here. Plus it was faster for me to add client-side than to put it on your backlog with Evan.
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.
Does that endpoint exist? I get a 404 when I try it.
No, we would have to add it.
Given the Agent will be responsible for stitching together pieces of information in the future,
I wonder how much logic the Agent must really have for the config. I'm afraid that if we go too far we block users from using their config management of choice. If the config consists of plain files users can manipulate them as they like. However, if they must use another tool it becomes more complicated. I'm probably missing a lot here. Do we have a document on future plans?
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.
Users will always be able to use whatever software they want to configure the Agent. This PR is just for the turn-key get-started install script on Grafana Cloud; we will never have a hard dependency on building a tool that must be used for configuring the Agent.
# retrieve_config downloads the config file for the Agent and prints out its | ||
# contents to stdout. | ||
retrieve_config() { | ||
grafana-agentctl cloud-config -u "${GCLOUD_STACK_ID}" -p "${GCLOUD_API_KEY}" || fatal 'Failed to retrieve config' |
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 probably miss something but why not use curl ${API_ENDPOINT}/stacks/${GCLOUD_STACK_ID}/agent.yaml
instead? The GCom client seems to be overhead unless we'll add more logic I am not aware of.
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 implementation of cloud-config
will probably change over time when integrations are decoupled from the API implementation. I'm comfortable maintaining a tiny package on the client side that's responsible for stitching together the final config file (which is where I think the logic should live anyway - the integrations API should, ideally, just give me config for integrations and remote_write info, and the Agent should be responsible for knowing how it should be configured outside of that scope).
It may turn out that we even remove the package later on, but I suspect it's also where the C&C client-side code will be written.
Co-authored-by: Jack Baldry <jack.baldry@grafana.com>
Co-authored-by: Jack Baldry <jack.baldry@grafana.com>
Co-authored-by: Karsten Jeschkies <k@jeschkies.xyz>
Tested on both rpm- and deb-based distros and verified this script is working as intended. I'm going to merge this now given it has two approvals, but @jeschkies feel free to reach out offline if we need to discuss more about this, I can backpedal later if necessary. |
* grafanacloud-specific install script * remove empty comment * Update cmd/agentctl/main.go Co-authored-by: Evan <10603766+hoenn@users.noreply.github.com> * appease the linter * Update production/grafanacloud-install.sh Co-authored-by: Jack Baldry <jack.baldry@grafana.com> * Update production/grafanacloud-install.sh Co-authored-by: Jack Baldry <jack.baldry@grafana.com> * Update production/grafanacloud-install.sh Co-authored-by: Karsten Jeschkies <k@jeschkies.xyz> * review feedback * agentctl: change to consistent command function naming * print success message after installing/starting agent service Co-authored-by: Evan <10603766+hoenn@users.noreply.github.com> Co-authored-by: Jack Baldry <jack.baldry@grafana.com> Co-authored-by: Karsten Jeschkies <k@jeschkies.xyz>
PR Description
Adds a Grafana Cloud-specific install script that does the following:
agentctl
command to generate the config file using an internal Grafana Cloud APIThis script can be used by setup instructions on Grafana Cloud.
Which issue(s) this PR fixes
Notes to the Reviewer
This currently won't work completely as
grafana-agentctl
isn't installed by the 0.9.0 binaries. To reproduce locally, install agentctl asgrafana-agentctl
. A 0.9.1 release is needed for this script to work fully on its own.PR Checklist