-
Notifications
You must be signed in to change notification settings - Fork 24
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
Validate derived column expressions during the planning stage #310
Comments
I think the best we could do in the validation stage would be a syntax check. We'd not be able to fully check that your DC is valid (as column may not yet exist if they're to be created in the same plan), but that is still much better than where we are today |
A full check (including DC) would be great, but I think a syntax check would probably catch >80% of the issues we've encountered so far. |
We have a use-case that might be relevant here, and that I could imagine steering us in a particular direction. Some of our teams have N SLIs that share the same "filter" query partial (the first 1/2 of the With this, we could let teams define DRYer queries like (pardon any pseudo-code mistakes here): locals {
sli_filter = "EXISTS($http.status_code)"
error_criteria = "LT($http.status_code, 500)"
latency_criteria = "LT($duration_ms, 1000)"
}
data "honeycombio_derived_column_expression" "error_sli" {
query = "IF(${local.sli_filter}, ${local.error_criteria})"
}
data "honeycombio_derived_column_expression" "latency_sli" {
query = "IF(${local.sli_filter}, ${local.latency_criteria})"
} |
We have something similar where we have a number of derived columns to tell us if a request is from one of our pen testers, our synthetic monitoring tool, or if something is client-induced. We write each of those out as their own derived column and then inline the (validated) expression into other derived columns: resource "honeycombio_derived_column" "sli_vizhelix" {
alias = "mode.dataengine.sli_vizhelix"
// Client read errors are not considered a failure even though they're reported
// as a 5xx error. These are due to client issues, which Flamingo has no
// control over.
expression = <<-EOT
IF(
AND(
${honeycombio_derived_column.is_vizhelix_related.expression},
NOT(${honeycombio_derived_column.is_ghost_inspector.expression}),
NOT(${honeycombio_derived_column.is_security_scanner.expression}),
NOT(${honeycombio_derived_column.helix_client_read_error.expression})
),
${honeycombio_derived_column.helix_success.expression}
)
EOT
description = "SLI for successfully Visualizing/Helixing on app.mode.com"
dataset = honeycombio_dataset.cdn_webapp_production.slug
} A nice thing about using multiple derived columns is that when validation fails we know better where the failure is. e.g. if something were wrong in the security scanner related portion of the expression, then creation of that column would fail and it would be clearer where the fix needs to be applied. |
That's slick. I didn't know you could chain derived columns together! |
Is your feature request related to a problem? Please describe.
I'm sure this is not a trivial problem, but it comes up often enough in our company that I figured I'd mention it here.
When creating SLIs we don't get failures (e.g. syntax) until the change is being applied. Planning doesn't surface things like:
Since we use Atlantis to apply changes via GitHub integration, this gets super weird for developers. They make changes and Atlantis shows them a "clean" plan, so they put it out for review. If the reviewer isn't eagle-eyed enough to catch the error, they approve the PR. The author then tells Atlantis to go ahead and apply, at which point it fails.
Now the developer needs to figure out what's wrong (can often involve going to the Honeycomb UI to create a derived column with the expression from their Terraform file to debug), fix it, re-submit it for review/plan, and then re-attempt to apply the change.
This might be easier if Honeycomb allowed "inline" expressions in the query builder (allowing the developer to skip the step of creating a derived column and cleaning it up later), which is something we've been requesting for years. Even better would be to have that and to have the Terraform provider fail at plan time. But I'd settle for either soonish if I couldn't have both right away.
Describe the solution you'd like
For
terraform plan
to raise errors for invalid derived column expressions.Describe alternatives you've considered
Only ever writing perfect derived column expressions. 😆
Additional context
None.
The text was updated successfully, but these errors were encountered: