This repository has been archived by the owner on Feb 9, 2022. It is now read-only.
Consider simplifying our Aspect configuration proto format. #565
Comments
|
I do agree that it will make some code simpler and we can stop using struct.
From a user's perspective However specifying Params in aspects and adapters is now different.
One uses 'quota_params' other uses just 'params'.
I am not sure if that is a win.
Using "iPhone 9";
… On Apr 17, 2017, at 8:41 PM, Martin Taillefer ***@***.***> wrote:
From an email thread:
On Mon, Apr 17, 2017 at 8:38 PM, Martin Taillefer ***@***.*** wrote:
Well let's see:
#1. Simpler Proto. Looking at the config proto is cleaner since the explicit types of the sub-messages are listed rather than having just a generic struct. Additionally, we no longer need a "kind" field since we can infer this by the message type. Finally, it moves the aspect config sub messages to live in the istio/api repo which is considerably more discoverable than its current location in istio/mixer/pkg/aspect/config/*. So we go from:
message Aspect {
string kind = 1;
string adapter = 2; // optional, allows specifying an adapter
// Struct representation of a proto defined by the aspect
google.protobuf.Struct params = 4;
}
to:
message Aspect {
string adapter = 2; // optional, allows specifying an adapter
oneof params {
ListsParams lists = 3;
... and all other aspect config blocks...
}
}
message ListParams {
...
}
#2. Simpler YAML. The kind field is no longer required. We thus go from:
aspects:
- kind: quotas
params:
quotas:
- descriptor_name: RequestCount
max_amount: 5
expiration: 1s
to
aspects:
- quota_params:
quotas:
- descriptor_name: RequestCount
max_amount: 5
expiration: 1s
#3. Simpler Code. Our code no longer needs to deal with the proto Struct stuff. The code is normalized and has better strong typing.
Basically, this the logical equivalent of switching a class field in Java from "object" to a strong type instead. It's easier to understand an API when you have a real type rather than "object", it enables the compiler to do type checking. It's easier to discover what you need to do with the field, etc.
On Mon, Apr 17, 2017 at 9:57 AM, Sven ***@***.*** wrote:
If/when we support more dynamic aspect addition, we can make the proto generated rather than predefined, avoiding the need to edit the protos.
I think the key thing is the developer experience and the simplicity of the system.
Can you give some real-world examples of the difference in config between what we're proposing now and what you'd like to see instead?
Much better to discuss with examples than in the abstract.
On Sat, Apr 15, 2017 at 4:47 AM, Martin Taillefer ***@***.*** wrote:
Yeah, that's what I proposed in the email that started this thread.
I don't think we want to inline the adapter config state there. It's valuable to have the adapter configs to have their own identity so they can be referenced from different aspects.
On Fri, Apr 14, 2017 at 5:28 PM, Louis Ryan ***@***.*** wrote:
If we're expecting folks who add aspects to be able to edit the protos (not entirely convinced of this but I could be persuaded) then couldn't we simply allow
message Aspect {
string kind = 1;
string adapter = 2; // optional, allows specifying an adapter
// Struct representation of a proto defined by the aspect
oneof {
LoggingAspectSpecificType logs = 1;
SomeOtherAspectSpecificType blah = 2;
}
}
If you want to tie the typed configurations to the adapter concretely then you could probably just elide the 'adapter' string and rely on the presence of an adapter specific config.
Also FWIW this golang/protobuf#199 (comment) might actually be useful here. Using DyanmicMessage to synthetically create a strongly typed proto from Adapter config on the fly.
On Fri, Apr 14, 2017 at 3:54 PM, Martin Taillefer ***@***.*** wrote:
The way the mixer is built, we don't expect that folks will introduce new aspects on the fly. Adding an aspect is a big deal overall. Someone that does this can readily extend the proto at the top of the world as well.
So far, I've heard:
Mandar doesn't think it's worth do use oneof for aspects if we can't also use it for adapters. So he's on the "everyone should suck in an equivalent way" plan :-).
Zack is concerned about uneven support for oneof in different languages. It's not clear to me whether this matters. These protos are used to parse config YAML files, not so much as a multi-language interchange format.
Sven pointing out that we wanted to keep things flexible.
To me, the proposal still sounds highly desirable for the sake of simplifying the model, both in our implementation and in what our users experience.
On Fri, Apr 14, 2017 at 3:42 PM, Sven ***@***.*** wrote:
Not sure if I'm following the discussion, but we didn't want to include any proto objects specific to a particular aspect or adapter in the config. The config should be general such that an installation with arbitrary aspects/adapters can work without having to build a custom config proto.
Alternatively we could generate a config proto specific to an installation, which gives the same effect, and for now we could hard code what that would look like for the aspects/adapters we've built so far. Depends on whether that gives a noticeable improvement to developer experience.
On Fri, Apr 14, 2017 at 3:39 PM, Martin Taillefer ***@***.*** wrote:
I don't think primitive fields are desirable here, all members of the oneof will be sub-messages.
On Fri, Apr 14, 2017 at 11:37 AM, Zack Butcher ***@***.*** wrote:
Another mark against oneof: we cannot use primitive types in a oneof, we have to use wrapper messages due to proto3's handling of default values. Otherwise it's impossible to tell in json (and yaml) which field of the union is set (since we synthesize the default values for primitives).
On Fri, Apr 14, 2017 at 10:01 AM, Mandar Jog ***@***.*** wrote:
+sven +lryan
On Fri, Apr 14, 2017 at 9:56 AM, Mandar Jog ***@***.*** wrote:
we will not need "kind" if we use oneof. It already encodes a "kind"
On Fri, Apr 14, 2017 at 9:44 AM, Sunny Gupta ***@***.*** wrote:
Do we need 'kind' field if we go with Oneof? If not, then it is one less thing to configure.
On Fri, Apr 14, 2017 at 9:11 AM, Douglas Reid ***@***.*** wrote:
Ha! This mirrors bits of the lunchtime discussion in MTV.
I believe the answer was that there was previously purposeful avoidance of oneof in the config protos. Perhaps it is time to revisit?
On Fri, Apr 14, 2017 at 8:46 AM, Martin Taillefer ***@***.*** wrote:
Seems like we could simplify the way we handle aspect protos using oneof. Instead of:
message Aspect {
string kind = 1;
string adapter = 2; // optional, allows specifying an adapter
// Struct representation of a proto defined by the aspect
google.protobuf.Struct params = 4;
}
What if we hoisted the config definitions from pkg/aspect/config and put them all inline right there using oneof:
message Aspect {
string kind = 1;
string adapter = 2; // optional, allows specifying an adapter
oneof params {
ListsParams lists = 3;
... and all other aspect config blocks...
}
}
message ListsParams {
bool blacklist = 1;
string check_expression = 2;
}
This would be a lot more discoverable and cleaner overall. The existing model pretends that we have a pluggable set of aspect kinds, which we really don't aspect kinds are a big deal, requiring plumbing in many places.
I bet this would simplify our code too.
WDYT?
--
Douglas Reid | Software Engineer | ***@***.*** | 650-318-1487
--
Thanks,
-SG
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Could we generate the adapter proto and make it also use a oneof? |
|
Hmmm. I was just writing a note in a separate issue regarding the fact I think we eventually need to support a model where somebody can create a custom version of the mixer binary which only includes the adapters they intend to use in their deployment. This is grounded in the simple security principle that the best way to avoid code being exploited is to remove said code. So if you're not going to use an adapter, you shouldn't have the code for the adapter at all. Given this, then it would seem natural that we could also generate the proto on the fly. Whomever builds the mixer would provide a list of adapters they want in the deployment, and the build system would generate the proto and would include only the adapters desired. |
|
The prior position was that we want adapters to be externally extensible, if we think it is no longer important then we can proceed. If we are ok with using oneof for both adapters and aspects we can generate the container protos to shove all these one offs in.This should be repeatable so the individual messages must be assigned unique and predictable ids. This will get rid of |
|
At this point, I don't think we're on a path to allow adapters to be
demand-loaded into the mixer. THis mechanism doesn't work on all platforms,
and is actually quite inefficient. They're not like a DLL being loaded into
a process, they're more like a "process sidecars" being loaded 'on the
side' with a full copy of all the library code. These plugins are massive,
and lead to poor cache efficiency since there is a ton of code duplication.
…On Tue, Apr 18, 2017 at 3:41 PM, mandarjog ***@***.***> wrote:
The prior position was that we want adapters to be externally extensible,
if we think it is no longer important then we can proceed.
If we are ok with using oneof for both adapters and aspects we can
generate the container protos to shove all these one offs in.This should be
repeatable so the individual messages must be assigned unique and
predictable ids.
This will get rid of struct which is very much welcome.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#565 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AVucHXT9iY0Mr9s7MqUsYH_1saVjDHzIks5rxTwTgaJpZM4M_1am>
.
|
|
I'm for getting rid of the oneofs and starting with a hand-written proto for both the aspect and adapters part, then doing generation of the protos as a second step, per the other issue. Seems fine to leave in beta though and have alpha use structs? |
|
We're done with Alpha coding changes. So yes, this is for beta.
…On Tue, Apr 18, 2017 at 5:14 PM, Sven Mawson ***@***.***> wrote:
I'm for getting rid of the oneofs and starting with a hand-written proto
for both the aspect and adapters part, then doing generation of the protos
as a second step, per the other issue.
Seems fine to leave in beta though and have alpha use structs?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#565 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AVucHTjhsjZc0JhA4IVgrMC9_Jxq0DJhks5rxVHegaJpZM4M_1am>
.
|
|
Out of date with reality. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
From an email thread:
On Mon, Apr 17, 2017 at 8:38 PM, Martin Taillefer mtail@google.com wrote:
Well let's see:
#1. Simpler Proto. Looking at the config proto is cleaner since the explicit types of the sub-messages are listed rather than having just a generic struct. Additionally, we no longer need a "kind" field since we can infer this by the message type. Finally, it moves the aspect config sub messages to live in the istio/api repo which is considerably more discoverable than its current location in istio/mixer/pkg/aspect/config/*. So we go from:
message Aspect {
string kind = 1;
string adapter = 2; // optional, allows specifying an adapter
// Struct representation of a proto defined by the aspect
google.protobuf.Struct params = 4;
}
to:
message Aspect {
string adapter = 2; // optional, allows specifying an adapter
oneof params {
ListsParams lists = 3;
... and all other aspect config blocks...
}
}
message ListParams {
...
}
#2. Simpler YAML. The kind field is no longer required. We thus go from:
to
#3. Simpler Code. Our code no longer needs to deal with the proto Struct stuff. The code is normalized and has better strong typing.
Basically, this the logical equivalent of switching a class field in Java from "object" to a strong type instead. It's easier to understand an API when you have a real type rather than "object", it enables the compiler to do type checking. It's easier to discover what you need to do with the field, etc.
On Mon, Apr 17, 2017 at 9:57 AM, Sven sven@google.com wrote:
If/when we support more dynamic aspect addition, we can make the proto generated rather than predefined, avoiding the need to edit the protos.
I think the key thing is the developer experience and the simplicity of the system.
Can you give some real-world examples of the difference in config between what we're proposing now and what you'd like to see instead?
Much better to discuss with examples than in the abstract.
On Sat, Apr 15, 2017 at 4:47 AM, Martin Taillefer mtail@google.com wrote:
Yeah, that's what I proposed in the email that started this thread.
I don't think we want to inline the adapter config state there. It's valuable to have the adapter configs to have their own identity so they can be referenced from different aspects.
On Fri, Apr 14, 2017 at 5:28 PM, Louis Ryan lryan@google.com wrote:
If we're expecting folks who add aspects to be able to edit the protos (not entirely convinced of this but I could be persuaded) then couldn't we simply allow
message Aspect {
string kind = 1;
string adapter = 2; // optional, allows specifying an adapter
// Struct representation of a proto defined by the aspect
oneof {
LoggingAspectSpecificType logs = 1;
SomeOtherAspectSpecificType blah = 2;
}
}
If you want to tie the typed configurations to the adapter concretely then you could probably just elide the 'adapter' string and rely on the presence of an adapter specific config.
Also FWIW this golang/protobuf#199 (comment) might actually be useful here. Using DyanmicMessage to synthetically create a strongly typed proto from Adapter config on the fly.
On Fri, Apr 14, 2017 at 3:54 PM, Martin Taillefer mtail@google.com wrote:
The way the mixer is built, we don't expect that folks will introduce new aspects on the fly. Adding an aspect is a big deal overall. Someone that does this can readily extend the proto at the top of the world as well.
So far, I've heard:
Mandar doesn't think it's worth do use oneof for aspects if we can't also use it for adapters. So he's on the "everyone should suck in an equivalent way" plan :-).
Zack is concerned about uneven support for oneof in different languages. It's not clear to me whether this matters. These protos are used to parse config YAML files, not so much as a multi-language interchange format.
Sven pointing out that we wanted to keep things flexible.
To me, the proposal still sounds highly desirable for the sake of simplifying the model, both in our implementation and in what our users experience.
On Fri, Apr 14, 2017 at 3:42 PM, Sven sven@google.com wrote:
Not sure if I'm following the discussion, but we didn't want to include any proto objects specific to a particular aspect or adapter in the config. The config should be general such that an installation with arbitrary aspects/adapters can work without having to build a custom config proto.
Alternatively we could generate a config proto specific to an installation, which gives the same effect, and for now we could hard code what that would look like for the aspects/adapters we've built so far. Depends on whether that gives a noticeable improvement to developer experience.
On Fri, Apr 14, 2017 at 3:39 PM, Martin Taillefer mtail@google.com wrote:
I don't think primitive fields are desirable here, all members of the oneof will be sub-messages.
On Fri, Apr 14, 2017 at 11:37 AM, Zack Butcher zbutcher@google.com wrote:
Another mark against oneof: we cannot use primitive types in a oneof, we have to use wrapper messages due to proto3's handling of default values. Otherwise it's impossible to tell in json (and yaml) which field of the union is set (since we synthesize the default values for primitives).
On Fri, Apr 14, 2017 at 10:01 AM, Mandar Jog mjog@google.com wrote:
+sven +lryan
On Fri, Apr 14, 2017 at 9:56 AM, Mandar Jog mjog@google.com wrote:
we will not need "kind" if we use oneof. It already encodes a "kind"
On Fri, Apr 14, 2017 at 9:44 AM, Sunny Gupta guptasu@google.com wrote:
Do we need 'kind' field if we go with Oneof? If not, then it is one less thing to configure.
On Fri, Apr 14, 2017 at 9:11 AM, Douglas Reid dougreid@google.com wrote:
Ha! This mirrors bits of the lunchtime discussion in MTV.
I believe the answer was that there was previously purposeful avoidance of oneof in the config protos. Perhaps it is time to revisit?
On Fri, Apr 14, 2017 at 8:46 AM, Martin Taillefer mtail@google.com wrote:
Seems like we could simplify the way we handle aspect protos using oneof. Instead of:
message Aspect {
string kind = 1;
string adapter = 2; // optional, allows specifying an adapter
// Struct representation of a proto defined by the aspect
google.protobuf.Struct params = 4;
}
What if we hoisted the config definitions from pkg/aspect/config and put them all inline right there using oneof:
message Aspect {
string kind = 1;
string adapter = 2; // optional, allows specifying an adapter
oneof params {
ListsParams lists = 3;
... and all other aspect config blocks...
}
}
message ListsParams {
bool blacklist = 1;
string check_expression = 2;
}
This would be a lot more discoverable and cleaner overall. The existing model pretends that we have a pluggable set of aspect kinds, which we really don't aspect kinds are a big deal, requiring plumbing in many places.
I bet this would simplify our code too.
WDYT?
--
Douglas Reid | Software Engineer | dougreid@google.com | 650-318-1487
--
Thanks,
-SG
The text was updated successfully, but these errors were encountered: