2018

D

2018

D

Does this actually work in the Mixer? Are we in fact delivering EmailAddress, URI, and DNSName using those types? Or are these delivered as type "string"?

I guess this code will work no matter.

As per the test, we are passing EmailAddress https://github.com/istio/istio/blob/master/mixer/template/sample/template.gen_test.go#L1049

I don't quite remember but there was something that we still need to polish on that front of EmailAddress and Url. Will look into it separately, as this code is unrelated to that.

This is actually quite slow, and produces output with spaces which I think would be best to avoid. Could you lift the time formatting code from pkg/log for this?

Fair point. followup question :
So, when operator is writing overriding dimensions for a label of type time.Time, how would they write a string version of it ? Inside adapter config, the adapter developer should explicitly mention what string format of time should be written by operator when writing override dimensions, and therefore adapter developer should exactly know how are we stringifying time.Time in this function. Therefore, I was thinking using default String() would help keep both Operator (writing override label value) and adapter dev(documenting operator config for dimensions field) on the same page.

If we use code from pkg/log, will operator be able to write string form of time.Time that we are generating there ?

The format we use in pkg/log corresponds to RFC8601 which is a pretty widely used standard, so I think we're covered there. Note of course that's it's highly unlikely that overrides would ever be written against a timestamp value used with this function, since it would require an exact match. For time-based overrides, the adapter would actually want to treat the dimension as a timestamp and probably do some range comparison instead of equality matches.

Note that the operator doesn't know what the standard Go timestamp serialization looks like. But they do know what ISO8601 looks like.

Could you mention in this comment that this is specifically designed for ValueType.

D

D

D

D

As per the test, we are passing EmailAddress https://github.com/istio/istio/blob/master/mixer/template/sample/template.gen_test.go#L1049

I don't quite remember but there was something that we still need to polish on that front of EmailAddress and Url. Will look into it separately, as this code is unrelated to that.

Fair point. followup question :
So, when operator is writing overriding dimensions for a label of type time.Time, how would they write a string version of it ? Inside adapter config, the adapter developer should explicitly mention what string format of time should be written by operator when writing override dimensions, and therefore adapter developer should exactly know how are we stringifying time.Time in this function. Therefore, I was thinking using default String() would help keep both Operator (writing override label value) and adapter dev(documenting operator config for dimensions field) on the same page.

If we use code from pkg/log, will operator be able to write string form of time.Time that we are generating there ?

The format we use in pkg/log corresponds to RFC8601 which is a pretty widely used standard, so I think we're covered there. Note of course that's it's highly unlikely that overrides would ever be written against a timestamp value used with this function, since it would require an exact match. For time-based overrides, the adapter would actually want to treat the dimension as a timestamp and probably do some range comparison instead of equality matches.

Note that the operator doesn't know what the standard Go timestamp serialization looks like. But they do know what ISO8601 looks like.

Few things here:

• Does the word "String" belong in this name? That seems like an implementation detail of the function, doesn't it? So maybe ValueEquals?

• It's not clear to me how integers, booleans, double, timestamp, duration values are compared here. Would these be handled automatically by the "a == b" check, or will they fall through and be compared as strings? If the "a == b" doesn't do it for those primitive types, we should have a type switch statement here instead and handle the comparison that way, without needing to convert everything to a string first.

• Does converting a string map to text using %v actually produce a deterministic output? That is, given that maps provide non-deterministic iteration semantics, it's not obvious that the stringification code bothers to sort anything before output. That would make comparison using stringification unreliable.

• I think String being part of the name, means it is comparing stringified form of two values, therfore "true" == . true, or "1.2.3.4" == net.IP("1.2.3.4"). If we do ValueEquals, then IMHO, just from the name, it would not imply "1.2.3.4" == net.IP("1.2.3.4"). WDYT ?

• You are right, and I have tried to call it out in the comment as well. If one of them is not string, then we do stringification first and then compare. It seems if I do "a == b", the code panics at runtime if the two values are IPAddress. Ideally I would like to do something like this:

if a.(type) == b.(type) {
   return a == b
}
.. rest of the existing code.

Unfortunately, there is no way to compare types in go except doing reflection. At least I couldn't find it.
Therefore, I resorted to unoptimized stringfy and compare for that case, instead of going with reflect on type, compare types and then compare values. Suggestions ??

• Hmm. Good catch. I will look into this after SVL trip by Thursday. I will need to check with "%v" does. Even if it does not give deterministic output, I would imagine we would want to do the right thing in our StringEquals ?

Hmmm, my comments were based on the previous commit, I made them yesterday and somehow they were sent today. Strange.

IS the name StringEquals appropriate? Maybe ValueEquals?

This gives a non-deterministic result since map iteration order is non-deterministic. You'll need to accumulate the keys in a slice, sort the slice, and iterate the slice.

Also, you can use pool.GetBuffer here to avoid allocating the byte.Buffer

Do we care if this is non-deterministic? Only during StringEquals we need to ensure the comparison is order agnostic, which we are doing. This string is just to saving in DB or printing. I have a test for this https://github.com/istio/istio/pull/3391/files/48bb5457e0a1775cfb50109b53a8a00e062e6097#diff-4f7b39f5e9706d4bc1c336abea02aecfR86

Will use the pool.

I think I'd feel better about deterministic output, since we don't know the scenario where the string will actually be used. It's unlikely to matter, but just for safety's sake, I think it'd be better to sort.

Regarding deterministic output, I was mainly concerned about the extra work of sorting during Stringify, since it is going to be commonly used by adapters. Well, I guess, safety first than performance.

Getting close :-)

You need to do a pool.PutBuffer before returning so put the buffer back into the pool.

Dang it. fixed