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

Query values converted to strings #27

Closed
fitzoh opened this issue Aug 16, 2020 · 7 comments · Fixed by #29
Closed

Query values converted to strings #27

fitzoh opened this issue Aug 16, 2020 · 7 comments · Fixed by #29
Labels

Comments

@fitzoh
Copy link
Contributor

fitzoh commented Aug 16, 2020

Creating a board using this honeycombio_query:

data "honeycombio_query" "db_heatmap" {
  calculation {
    op     = "HEATMAP"
    column = "db.duration"
  }
  filter {
    column = "db.duration"
    op     = ">"
    value  = 500
  }
}

ends up yielding this query:
Annotation 2020-08-15 232920

(Note that the duration is quoted)

This results in no data being shown, while data is returned if the value is converted to a numeric value.

@yvrhdn
Copy link
Contributor

yvrhdn commented Aug 16, 2020

The provider casts filter.value to a string, before assigning it: https://github.com/kvrhdn/terraform-provider-honeycombio/blob/main/honeycombio/data_source_query.go#L173
In the schema it's also declared as TypeString.

But in go-honeycombio filter value is a interface{}: https://github.com/kvrhdn/go-honeycombio/blob/main/query_spec.go#L64

So, we'll have to figure out whether it's possible to have a dynamic type in Terraform.

@yvrhdn yvrhdn added the bug label Aug 16, 2020
@fitzoh
Copy link
Contributor Author

fitzoh commented Aug 16, 2020

Does it make sense to add mutually exclusive value_string and value_numeric fields?

Feels kind of gross, but probably the most straightforward option.

@yvrhdn
Copy link
Contributor

yvrhdn commented Aug 16, 2020

Yeah, I'd like to avoid having to do that 😕 but it might be the easiest way...
I'll have to investigate the Terraform type system a bit more first.

@fitzoh
Copy link
Contributor Author

fitzoh commented Aug 16, 2020

From a quick look around it doesn't look like there's a way to specify that a field is a string or numeric.

My other thought was we could maybe have the go client convert it based on the operator.
This would probably work for comparison operators, but would fail on =/!= (those could be used with either string or numeric values).

The best option would probably be on the honeycomb side (we always send a string, honeycomb converts to an appropriate value for the field), but I think explicit value/numeric fields might be the best option for now.

But yeah, if you find a better option in the terraform type system that works too.

@yvrhdn
Copy link
Contributor

yvrhdn commented Aug 16, 2020

After playing a bit with the UI, it seems that the UI interprets the filter value based upon the type of the column. There are four types (string, integer, float, boolean) which can be set in the dataset settings.

For example, the result of entering 0.5 with the various types:

type of the column what you type what the UI shows after pressing enter
string 0.5 "0.5"
integer 0.5 0
float 0.5 0.5
boolean 0.5 input is not accepted

While this feels intuitive, it is impossible to recreate in the Terraform provider:

  • we don't have access to the column type using the API, so it's impossible to determine which conversion is appropriate
  • additionally, the API ignores the column type and always uses the type of the value in the JSON payload. If you send a string value, the query will use a string as filter value, even if the column has type float (as shown in the first comment).

The type system of Terraform is also a limiting factor: filter.value is declared as TypeString so even if you set value = 0.5 the Terraform SDK will always return a string. It's not possible to determine whether the user originally used a string or a number.
There is some work in the pipeline to improve the Terraform type system, starting with the v2 SDK (which dropped support for Terraform 0.11). The DynamicPseudoType (hashicorp/terraform-plugin-sdk#248) might fix this issue, but it is expected in a later release of the SDK v2.x.

Solutions:

  1. The best IMO: the Honeycomb API interprets the filter value using the same rules as the UI. I.e. if you send a string "0.5" but the type is float, the API should be able to convert this to a float 0.5.
  2. Intuitive but also a bit risky: the Terraform provider tries to interpret the value by converting it to a float or integer. I.e. if you enter "0.5" we try to convert this as a float and if it succeeds, we send a float to the API. This might cause weird behavior if the type is not always clear, for instance version = "0.1" should probably not be interpreted as a float.
  3. Add a property value_type (which defaults to string):
filter {
  column = "app.tenant"
  op     = "="
  value  = "SpecialTenant" // no value_type since string is the default
}
filter {
  column     = "duration_ms"
  op         = ">"
  value      = 1000
  value_type = "int"
  1. Provide a version of value for every possible type:
filter {
  column       = "app.tenant"
  op           = "="
  value_string = "SpecialTenant"
}
filter {
  column    = "duration_ms"
  op        = ">"
  value_int = 1000

@yvrhdn
Copy link
Contributor

yvrhdn commented Aug 16, 2020

Created a new topic on the Hashicorp forum to ask about fields with dynamic types
https://discuss.hashicorp.com/t/dynamic-type-in-schema/12919

@fitzoh
Copy link
Contributor Author

fitzoh commented Aug 17, 2020

Definitely agree on #1 being the best option, not a fan of #2 due to ambiguity.

I think I like 4 slightly more due to it being more explicit/harder to miss, but don't have strong objections to 3.

I would say go for 3 or 4 for now, put in a request for API support for 1 and plan on a major release/breaking change once API support in place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants