Add feature flag #56

AndersonQ · 2023-02-01T10:59:05Z

closes #55
relates elastic/elastic-agent#2185

Testing notes

See testing notes on elastic/elastic-agent#2218.

elasticmachine · 2023-02-01T11:02:46Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-03-09T11:50:52.991+0000
Duration: 3 min 6 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elastic-agent-client.proto

pkg/client/client_v2.go

michalpristas · 2023-02-07T13:09:30Z

pkg/client/unit.go

-// Expected returns the expected state and config for the unit.
-func (u *Unit) Expected() (UnitState, UnitLogLevel, *proto.UnitExpectedConfig) {
+// Expected returns the expected state, log leve, features and config for the unit.
+func (u *Unit) Expected() (UnitState, UnitLogLevel, *proto.Features, *proto.UnitExpectedConfig) {


changing ExpectedConfig returned value, do we have PRs on all repos using client, so we're aligned? endpoint needs to be aware as well

No, but they don't have to update right away. Anyway it's just a return value to ignore, it's easy to do

This is just the Go client, only changes in the .proto file affect endpoint. That said this feature will not fully work unless we align with endpoint, cloudbeat, and APM as well.

I'm also not sure the features makes sense as a per unit return value here, since the feature flags are not actually per unit.

A separate Features() channel seems like a better approach, that way users of this don't need to filter out and separately handle the feature changes from the unit changes.

that's what we were talking with anderson in the mornign. providing a separate channel to watch inside checkin loop

Yep, what is done now I think breaks the client API contract #56 (comment)

We need to change this before we can merge it.

I agree, it's a per component configuration. Sneaking it into the current config seemed fine to me before I understood how the protocol is actually implemented. Also I didn't want to make any big change to try to make to 8.7.

However I do think a change in the feature flag is ultimately a change in config. Pursuing the idea of making it already ready for future features that might be behind a feature flag, I can imagine a scenario where we'd need to reload components (in the generic sense, not as a beat) to properly apply the feature flags. And I don't know if it'd be a good idea having 2 flows reloading components, one for config and another for feature flags

elastic-agent-client.proto

cmacknz

I don't think the feature flags have been handled in the right place in the client code.

cmacknz · 2023-02-07T13:55:52Z

pkg/client/unit.go

 	if u.configIdx != cfgIdx {
 		u.configIdx = cfgIdx
-		if !gproto.Equal(u.config.GetSource(), cfg.GetSource()) {
+		if !gproto.Equal(u.config.GetSource(), cfg.GetSource()) ||
+			!gproto.Equal(u.features, features) {


I think this will be a source of bugs, because we will now send unchanged unit configurations to clients of this code and they will need to filter them out.

The intent of the elastic agent client is to only send unit modified events to a client of the package when the actual unit configuration changed. This breaks that contract, and will send unchanged unit configurations if only the feature flags have been modified.

brian-mckinney

My review applies only to the .proto changes. Adding the Features as a top level field will work better for Endpoint as well. 👍

cmacknz · 2023-02-08T18:23:55Z

The team spoke about what we should do in the Go client this morning and concluded:

When a feature changes, we should trigger a UnitModified event for every unit.
The UnitModified event should be extended to included the set of changes that triggered the event. The set of possible change triggers right now is a change to a Feature, a change to the log level, or a change to the Unit configuration. A single UnitModified event must be associated with at least one of these events, but may also include all of them together if all of them have changed.

We agreed that the changes to the .proto files are good as they are.

cmacknz

API design is looking good, minus a few nits and missing tests.

elastic-agent-client.proto

magefile.go

pkg/client/client_v2.go

pkg/client/unit.go

pkg/client/client_v2.go

cmacknz · 2023-02-10T20:08:43Z

pkg/client/client_v2_test.go

+			case change := <-validClient.UnitChanged():
+				for _, t := range change.Triggers {
+					if t == TriggerFeature {
+						gotFQDN = change.Features.FQDN.Enabled


Is it worth having a trigger per feature?

It seems like as soon as we support more than one feature we will force every user of this package to have to iterate over all of them to find what changed. Having something like TriggerFQDNFeature instead seems like it would be more convenient, at the cost of an additional enum entry per feature.

cmacknz · 2023-02-10T20:10:13Z

pkg/client/client_v2_test.go

 	go func() {
 		for {
 			select {
 			case <-ctx.Done():
 				return
-			case change := <-client.UnitChanges():
+			case change := <-client.UnitChanged():
+				for _, t := range change.Triggers {


We should be testing that each of the trigger types works, not just the feature triggers.

pkg/client/unit.go

pkg/client/client_v2.go

elastic-agent-client.proto

pkg/client/artifact_test.go

pkg/client/client_v2.go

elastic-agent-client.proto

pkg/client/client_v2.go

blakerouse · 2023-02-14T19:34:20Z

pkg/client/client_v2.go

+	errCh              chan error
+	changesCh          chan UnitChanged
+	featuresMu         sync.Mutex
+	features           Features


Going to need to add the featuresIdx here as well. I would start the initial index at 1.

There is a reason for starting it at 1, and that is backwards compatibility. With older clients, I would modify the Elastic Agent to read the observed features idx at 0, as the client doesn't understand features and we should not trigger an unsettle when it doesn't match, because it will never match. If it gets a 1, then it knows its a client that supports it but doesn't know about it. Or you add a boolean field in the observed message to signal that the client supports features.

Unless I'm missing something, I'm not sure we need features (and related fields) here on the clientV2 struct at all, since features is a field on the Unit struct (similar to logLevel).

fearful-symmetry · 2023-02-27T23:27:19Z

pkg/client/unit.go

+}
+
+// Expected returns the expected state, log level, features and config for the unit.
+func (u *Unit) Expected() Expected {


This definitely falls outside the scope of this PR, but I think we're really overloading the Expected() call. This adds a lot of complexity to clients, which now have to figure out what's actually changed every time a unit comes across the V2 interface. The shipper already does this, as some additional logic is required to see if a unit update is just a log level change. In the future, we might want to break out some of the data passed in the Expected struct into separate channels. It's probably too late to do that here though, and I don't wanna delay this PR any more.

I could be misunderstanding but I'm not sure much has changed from a "contract" perspective here? It looks like, before this PR Expected() was returning three values whereas it's returning four values now, but encapsulated in an Expected struct object. So callers will need to be updated whenever they bump up the version of their elastic-agent-client dependency.

That said, I see your point about Expected() returning too much information (even prior to this PR), which burdens the caller with having to figure out what's actually changed in a unit. I've created #62 to discuss and track this work.

ycombinator · 2023-03-07T17:25:22Z

There are a number of cleanup changes in this PR that aren't strictly related to the feature flag implementation. To make it easier to review this PR (for selfish reasons 😄), I've extracted the cleanup changes into their own PR: #61.

Once that cleanup PR is merged, I'll rebase this one on main so it ends up containing only FQDN-related changes.

…ide UnitChanged

michalpristas · 2023-03-09T09:03:48Z

pkg/client/client_v2.go


 const (
 	// UnitChangedAdded is when a new unit is added.
-	UnitChangedAdded UnitChangedType = 1
+	UnitChangedAdded UnitChangedType = iota // unit_added


having added as a default value for ChangedType, on purpose?

Yeah, good catch. I'm not sure this is a good idea either. And, besides, this being an exported symbol, the change from 1 => 0 (and thereon) could break existing consumers of this package? I'll reset this to 1.

michalpristas · 2023-03-09T09:05:53Z

pkg/client/client_v2.go

+	TriggeredNothing Trigger = 0 // nothing_triggered
+
+	// TriggeredConfigChange indicates a change in config triggered the change.
+	TriggeredConfigChange Trigger = 1 << iota // config_change_triggered


can you leave a comment that event can have multiple triggers and this is a mask so nobody accidentally changes this

michalpristas

i think it looks good. alex had a good comment about Expected though

ycombinator · 2023-03-09T12:05:43Z

alex had a good comment about Expected though

++ addressed it in the same thread; tl;dr: see #62.

AndersonQ marked this pull request as ready for review February 1, 2023 14:01

AndersonQ self-assigned this Feb 1, 2023

cmacknz reviewed Feb 1, 2023

View reviewed changes

elastic-agent-client.proto Outdated Show resolved Hide resolved

cmacknz requested review from cmacknz and blakerouse February 1, 2023 20:41

cmacknz added the Team:Elastic-Agent Label for the Agent team label Feb 1, 2023

blakerouse requested changes Feb 6, 2023

View reviewed changes

elastic-agent-client.proto Outdated Show resolved Hide resolved

AndersonQ force-pushed the 55-feature-flag branch from 7cbf7a7 to 9a8c3b8 Compare February 7, 2023 09:30

michalpristas reviewed Feb 7, 2023

View reviewed changes

pkg/client/client_v2.go Outdated Show resolved Hide resolved

michalpristas reviewed Feb 7, 2023

View reviewed changes

cmacknz reviewed Feb 7, 2023

View reviewed changes

elastic-agent-client.proto Show resolved Hide resolved

cmacknz requested changes Feb 7, 2023

View reviewed changes

cmacknz mentioned this pull request Feb 7, 2023

Report FQDN on host.name enabled by a feature flag elastic/elastic-agent#2185

Closed

cmacknz requested a review from brian-mckinney February 7, 2023 18:50

brian-mckinney approved these changes Feb 7, 2023

View reviewed changes

AndersonQ force-pushed the 55-feature-flag branch from 2e3bbb6 to 9a742b9 Compare February 8, 2023 10:56

cmacknz mentioned this pull request Feb 8, 2023

Update the shipper to work with the elastic-agent elastic/elastic-agent-shipper#185

Merged

AndersonQ force-pushed the 55-feature-flag branch from df32207 to 48dcd24 Compare February 10, 2023 17:03

cmacknz reviewed Feb 10, 2023

View reviewed changes

blakerouse reviewed Feb 14, 2023

View reviewed changes

AndersonQ force-pushed the 55-feature-flag branch 3 times, most recently from 30ce399 to 07faf31 Compare February 27, 2023 19:00

fearful-symmetry reviewed Feb 27, 2023

View reviewed changes

AndersonQ force-pushed the 55-feature-flag branch 2 times, most recently from eab0963 to 8ff7d95 Compare March 1, 2023 20:15

ycombinator mentioned this pull request Mar 7, 2023

Upgrade to Go 1.18 + minor cleanup #61

Merged

AndersonQ and others added 10 commits March 7, 2023 13:12

add and fix tests, some PR and small changes

201094e

remove stringer usage

feba3f0

rebase and go mod tidy

40bdac0

fix test

39ca174

tests fixed

35b9f3b

add GoString and don't send nil FeatureFlags

d8975ea

.

56d8b4a

making linter happy

acba3c0

Remove stringer setup from magefile

1bb9fcf

Fixing rebase errors

b9b4996

ycombinator force-pushed the 55-feature-flag branch from da6bd64 to b9b4996 Compare March 7, 2023 18:14

ycombinator added 5 commits March 7, 2023 13:17

Fixing duplicate mention in comment

09474ef

Pass unit features to unit constructor

ee7eef8

Remove features information from client

29534aa

Remove features from UnitChanged as they are already part of Unit ins…

02662bf

…ide UnitChanged

Fixing up tests

d7d45d9

ycombinator requested review from michalpristas, cmacknz and blakerouse March 8, 2023 21:57

ycombinator added enhancement New feature or request backport-v8.7.0 Automated backport with mergify labels Mar 8, 2023

michalpristas reviewed Mar 9, 2023

View reviewed changes

michalpristas approved these changes Mar 9, 2023

View reviewed changes

ycombinator added 2 commits March 9, 2023 06:45

Start UnitChangedType constants' numbering at 1

b70d208

Add comments about Trigger values being bitmasks

5cfba1d

ycombinator mentioned this pull request Mar 9, 2023

Report FQDN on host.name enabled by a feature flag elastic/beats#34456

Merged

6 tasks

jlind23 merged commit db7f3a0 into elastic:main Mar 9, 2023

ycombinator mentioned this pull request Mar 11, 2023

Send feature flags information as part of observed state in the next checkin #63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature flag #56

Add feature flag #56

AndersonQ commented Feb 1, 2023 •

edited by ycombinator

Loading

elasticmachine commented Feb 1, 2023 •

edited

Loading

Build stats

michalpristas Feb 7, 2023

AndersonQ Feb 7, 2023

cmacknz Feb 7, 2023

cmacknz Feb 7, 2023

michalpristas Feb 7, 2023

cmacknz Feb 7, 2023

AndersonQ Feb 7, 2023

cmacknz left a comment

cmacknz Feb 7, 2023

brian-mckinney left a comment

cmacknz commented Feb 8, 2023 •

edited

Loading

cmacknz left a comment

cmacknz Feb 10, 2023

cmacknz Feb 10, 2023

blakerouse Feb 14, 2023

ycombinator Mar 8, 2023

fearful-symmetry Feb 27, 2023

ycombinator Mar 9, 2023 •

edited

Loading

ycombinator commented Mar 7, 2023

michalpristas Mar 9, 2023 •

edited

Loading

ycombinator Mar 9, 2023

ycombinator Mar 9, 2023

michalpristas Mar 9, 2023

ycombinator Mar 9, 2023

michalpristas left a comment

ycombinator commented Mar 9, 2023

Add feature flag #56

Add feature flag #56

Conversation

AndersonQ commented Feb 1, 2023 • edited by ycombinator Loading

Testing notes

elasticmachine commented Feb 1, 2023 • edited Loading

💚 Build Succeeded

Build stats

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmacknz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-mckinney left a comment

Choose a reason for hiding this comment

cmacknz commented Feb 8, 2023 • edited Loading

cmacknz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

ycombinator commented Mar 7, 2023

michalpristas Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalpristas left a comment

Choose a reason for hiding this comment

ycombinator commented Mar 9, 2023

AndersonQ commented Feb 1, 2023 •

edited by ycombinator

Loading

elasticmachine commented Feb 1, 2023 •

edited

Loading

cmacknz commented Feb 8, 2023 •

edited

Loading

ycombinator Mar 9, 2023 •

edited

Loading

michalpristas Mar 9, 2023 •

edited

Loading