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

Add Buildkite Org to Cloudwatch Metrics as a Dimension to support multiple orgs per AWS account #510

Merged
merged 13 commits into from Jan 29, 2019

Conversation

lox
Copy link
Contributor

@lox lox commented Dec 28, 2018

We removed BuildkiteOrgSlug when we introduced the new Agent Metrics API. The idea was to reduce the number of required parameters, but we also removed the piece of data that was needed to support multiple Buildkite Orgs per AWS account, as otherwise the metrics are disambiguated only by queue.

This adds back BuildkiteOrgSlug with a default of `` and then adds an Org dimension to the metrics we publish to Cloudwatch.

Closes #452.

@lox lox requested a review from toolmantim December 28, 2018 22:02
@@ -93,6 +94,11 @@ Parameters:
- edge
Default: "stable"

BuildkiteOrgSlug:
Description: Buildkite organization slug
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be unclear as to why it's needed, or what it does? It doesn't need to actually match your Buildkite org slug does it, it just scopes the metrics? It'd be so nice if we could just extract it from the agent token somehow 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to have it blank, and optional, rather than the string "none"?

Suggested change
Description: Buildkite organization slug
Description: Optional Buildkite organization slug (e.g. "my-org"). This only needs to be set when running stacks for different Buildkite organisations in a single AWS account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't think of an easy way to set up the conditionals everywhere else to support an empty one. I guess we could just publish an Org= dimension, but that seemed weirder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where do people see the dimension?

@lox
Copy link
Contributor Author

lox commented Jan 7, 2019

@toolmantim so despite adding the new dimension to the metric, I realized that we still need to filter on that dimension, which means we need to specify it in the stack still.

@toolmantim
Copy link
Contributor

I was wondering how you were going to make the stack aware of it @lox! I thought maybe using a Custom Resource Lambda function or something.

@lox
Copy link
Contributor Author

lox commented Jan 7, 2019

I could potentially do that, I just think this will be simpler.

@toolmantim
Copy link
Contributor

Okie dokie. We should update the org slug param description though. It's unclear what the "org slug" is, and that it doesn't actually even need to match the org slug (it's just a name spacing/scoping thing… it won't error if it's wrong, or the org name changes).

I think also we can pre-fill CloudFormation parameters from URL values now? We should update the in-app links so we can pre-fill the parameters for people.

Should I roll back the backend change?

@lox
Copy link
Contributor Author

lox commented Jan 7, 2019

Okie dokie. We should update the org slug param description though. It's unclear what the "org slug" is, and that it doesn't actually even need to match the org slug (it's just a name spacing/scoping thing… it won't error if it's wrong, or the org name changes).

It does need to match the org-slug! That's the metric that will be set in the backend. This change still relies on getting the org-slug from the metrics API change you made.

I think also we can pre-fill CloudFormation parameters from URL values now? We should update the in-app links so we can pre-fill the parameters for people.

🤷🏼‍♂️

Should I roll back the backend change?

Nah, let's roll with it. I think it will be a good building block.

@toolmantim
Copy link
Contributor

It does need to match the org-slug! That's the metric that will be set in the backend. This change still relies on getting the org-slug from the metrics API change you made.

Oh, I thought it was passed through to the Lambda from the stack! Nevermind then.

@lox
Copy link
Contributor Author

lox commented Jan 7, 2019

Oh, I thought it was passed through to the Lambda from the stack! Nevermind then.

It was originally, but I figured we'd just leave it with the metrics populating it. You do have a point that it means you need to get the org-slug correct, but that's probably a good idea anyway?

@toolmantim
Copy link
Contributor

I'd always err on "make it simpler for the users, even if it less-simple to implement" — but I don't know just how hard it is to require only the agent token.

One other question: what will happen for people that set up one stack, then need to set up a second one. Do they go and update the existing stack, add the org slug to scope it, apply (and hopefully the metrics work okay?)… then they create the second stack? Or do they just create the second stack (and have to remember to set the org scope?)

@lox
Copy link
Contributor Author

lox commented Jan 18, 2019

Darned if I can make it work anyway.

One other question: what will happen for people that set up one stack, then need to set up a second one. Do they go and update the existing stack, add the org slug to scope it, apply (and hopefully the metrics work okay?)… then they create the second stack? Or do they just create the second stack (and have to remember to set the org scope?)

They would need to update the first stack. The nice thing about always collecting the org in the metrics regardless of whether a stack filters on it is that the data is collected so that this update would work instantly.

@lox
Copy link
Contributor Author

lox commented Jan 29, 2019

@toolmantim this is working now, I'm pretty keen to merge it in as various folks have asked about it. Feels?

Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

Awesome. And the new parameter description is great too. Let's 🚀 it!

@lox lox merged commit 2f298e9 into master Jan 29, 2019
@lox lox deleted the add-org-to-cloudwatch-metrics branch January 29, 2019 08:38
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.

None yet

2 participants