Add leniency and type coercion for prom datasource options - #310
Add leniency and type coercion for prom datasource options#310jcolladokuri wants to merge 11 commits into
Conversation
iwysiu
left a comment
There was a problem hiding this comment.
I have a couple questions but overall looks good!
| if len(value) > maxLoggedValueLen { | ||
| value = value[:maxLoggedValueLen] + "…" | ||
| } | ||
| log.DefaultLogger.Warn("datasource jsonData value does not match its declared type", |
There was a problem hiding this comment.
can you pipe in the prometheus logger here so it has a little more context?
There was a problem hiding this comment.
is there anything specific from the additional context that you think would be helpful to add here? piping the prom logger has some trade-offs. Because these unmarshalling functions are called directly from encoding/json looking would have to live outside of the UnmarshalJSON functions and moved into ParsePromOptions, it requires additional parsing and we'd lose information about which path the specific value took (i.e. from int64 to string).
Would love to hear your thoughts on this
| coerced("int64", "string", data) | ||
| return nil | ||
| } | ||
| dropped("int64", "string", data) |
There was a problem hiding this comment.
I think that if we drop an invalid int64 here we actually set lenientInt64 to 0 (since its allocated and we don't return an unmarshaling error)
There was a problem hiding this comment.
Great callout, I have fixed this! A few things:
- unrelated to your comment but got rid of lenient int since it was only used by one field and the fronted already sets
seriesLimitto anumberanyway sofloat64more closely matches adouble - For your call out, I added a flag for whether a value was actually readable or not (
"10"vs"ten") and should be dropped. This way we are able to distinguish actual0values from when it should benil.
Let me know what you think!
Summary
This PR adds support for leniency on prom datasource settings types and type coercion where possible.
Before,
jsonDatawas treated and unmarshalled as genericmap[string]anythis allowed for wrongly typed values to be part of datasource configuration (i.e."true"instead oftrueStarting with #220, strict typing was enforced for all datasource config fields. For any existing customers that are using provisioning, this effectively broke their datasources.
This PR fixes that by adding LenientTypes, type coercion when possible, and if this fails, the field gets dropped instead of crashing the whole datasource. Any fields that were previously strict, remain strict (i.e.
httpMethod)Before (blocking on go unmarshalling):
After (letting the datasource health check go through):
Detailed summary
LenientBool: tries type coercion fromstringandfloat64LenientString: tries type coercion fromfloat64andboolLenientFloat64: tries type coercion fromstringLenientInt64: tries type coersion fromfloat64andstringLenientExemplarTraceIDDestinations: no type coercionFuture work
Right now this will help unblock customers who had provisioned datasources working under the previous generic jsonData. However, as a follow up, we should expose to customers that specific fields were coerced or dropped so that they could potentially correct it and we move towards a strict schema completely.
Testing
Created a prom provisioned datasource with the wrong types for different fields and ensure that the datasource still loaded, and had the right value where coercion was possible. (#311)