feat: add GetUserAttributeKeys by kakcy · Pull Request #1994 · bucketeer-io/bucketeer

kakcy · 2025-07-15T02:45:21Z

This pull request introduces a new feature to manage user attributes across environments, including API endpoint additions, caching enhancements, and related tests. The most important changes include the addition of a new API endpoint to retrieve user attribute keys, integration of a user attributes cache, and logic to process and store user attributes in the cache.

related to #686

API Enhancements:

Added a new API endpoint /v1/feature/user-attribute-keys in web-api.swagger.yaml to retrieve user attribute keys for a specified environment, including detailed response codes and descriptions.
Defined the featureGetUserAttributeKeysResponse schema in web-api.swagger.yaml to structure the API response for the new endpoint.

Caching and Service Updates:

Integrated a UserAttributesCache into the FeatureService in api.go, enabling storage and retrieval of user attribute keys. [1] [2]

Logic for User Attributes Processing:

Implemented the GetUserAttributeKeys method in user_attribute.go to retrieve user attribute keys from the cache, with validation and error handling.
Added logic in evaluation_events_evaluation_count_event_persister.go to extract user attributes from events and store them in the cache. [1] [2]

Testing:

Added unit tests for the GetUserAttributeKeys method in user_attribute_test.go, covering various scenarios such as empty environment ID, permission errors, cache errors, and successful responses.
Updated existing test cases in api_test.go to mock the UserAttributesCache and validate its integration. [1] [2] [3]

Copilot

Pull Request Overview

This PR adds support for a new GetUserAttributeKeys feature endpoint, including server-side logic, client stubs, caching integration, and tests.

Introduces new gRPC/HTTP RPC and protobuf messages for retrieving user attribute keys.
Implements caching and event-based extraction of user attributes in the subscriber.
Adds API handler, client mocks, JS stubs, Swagger spec, and unit tests for the new endpoint.

Reviewed Changes

Copilot reviewed 13 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
ui/web-v2/src/proto/feature/service_pb_service.js	Add JS client method for GetUserAttributeKeys
ui/web-v2/src/proto/feature/service_pb.js	Add JS proto definitions for request/response
proto/feature/service.proto	Define GetUserAttributeKeys messages and RPC
proto/feature/service.pb.gw.go	Generate HTTP handlers for new RPC
pkg/subscriber/processor/evaluation_events_evaluation_count_event_persister.go	Add updateUserAttributes to extract/cache attributes
pkg/subscriber/cmd/server/server.go	Wire UserAttributesCache into subscriber processor
pkg/feature/client/mock/client.go	Mock GetUserAttributeKeys in feature client
pkg/feature/api/user_attribute.go	Implement API endpoint logic for GetUserAttributeKeys
pkg/feature/api/user_attribute_test.go	Add unit tests for GetUserAttributeKeys
pkg/feature/api/api.go	Register UserAttributesCache in FeatureService
api-description/web-api.swagger.yaml	Document new endpoint and response schema
Makefile	Restrict e2e tests to gateway directory

Comments suppressed due to low confidence (4)

pkg/subscriber/processor/evaluation_events_evaluation_count_event_persister.go:494

[nitpick] Consider adding unit tests for updateUserAttributes to verify correct extraction and caching of user attributes across different event scenarios.

func (p *evaluationCountEventPersister) updateUserAttributes(envEvents environmentEventMap) {

proto/feature/service.proto:1655

The HTTP GET annotation should include the environment_id query parameter (e.g., "/v1/feature/user-attribute-keys?environmentId={environment_id}") to bind the request field correctly.

      get: "/v1/feature/user-attribute-keys"

api-description/web-api.swagger.yaml:5160

The swagger spec defines a 503 response but the proto HTTP options only specify up to a 500 response. Align status codes between the proto annotations and the swagger documentation.

        "503":

Makefile:251

[nitpick] Restricting the e2e target to only test/e2e/gateway may omit other end-to-end tests. Verify this aligns with intended coverage requirements.

	go test -v ./test/e2e/gateway/... -args \

Copilot · 2025-07-15T03:29:04Z

+	_, err := s.checkEnvironmentRole(
+		ctx, accountproto.AccountV2_Role_Environment_VIEWER,
+		req.EnvironmentId, localizer)
+	if err != nil {
+		s.logger.Error("Failed to get user attribute keys", zap.Error(err))
+		return nil, err
+	}
+


The check for empty EnvironmentId happens after permission validation, so missing-ID errors may be masked by permission errors. Consider validating req.EnvironmentId is non-empty before calling checkEnvironmentRole.

Suggested change

_, err := s.checkEnvironmentRole(

ctx, accountproto.AccountV2_Role_Environment_VIEWER,

req.EnvironmentId, localizer)

if err != nil {

s.logger.Error("Failed to get user attribute keys", zap.Error(err))

return nil, err

}

cre8ivejp · 2025-07-15T13:07:59Z

+	if req.EnvironmentId == "" {
+		dt, err := statusMissingID.WithDetails(&errdetails.LocalizedMessage{
+			Locale:  localizer.GetLocale(),
+			Message: localizer.MustLocalizeWithTemplate(locale.RequiredFieldTemplate, "environment_id"),
+		})
+		if err != nil {
+			return nil, statusInternal.Err()
+		}
+		return nil, dt.Err()
+	}


Because we have environments with empty IDs, we can't validate them.

cre8ivejp · 2025-07-15T13:25:09Z

 		// Increment the evaluation event count in the Redis
 		fails := p.incrementEnvEvents(envEvents)
+		// Save user attributes to cache
+		p.updateUserAttributes(envEvents)


Doing this synchronously will delay the data in the pubsub. Since this data is not essential and can be lost, it should be done asynchronously.

Also, as we discussed, we don't need to save all requests in Redis because 90% of the cases would have the same user attributes. This will increase the cost of the Redis Private service connection.
Please check the way we did it to save the last_used_info logic.

bucketeer/pkg/subscriber/processor/evaluation_events_evaluation_count_event_persister.go

Lines 148 to 154 in c7f6326

// Update the feature flag last-used cache

p.cacheLastUsedInfoPerEnv(envEvents)

updateEvaluationCounter(envEvents)

case <-ticker.C:

envEvents := p.extractEvents(batch)

// Update the feature flag last-used cache

p.cacheLastUsedInfoPerEnv(envEvents)

cre8ivejp · 2025-07-15T13:26:08Z

+	featureproto "github.com/bucketeer-io/bucketeer/proto/feature"
+)
+
+func TestGetUserAttributeKeys(t *testing.T) {


An e2e test for this API would be nice!

cre8ivejp · 2025-07-15T13:27:38Z

+					zap.String("environmentId", userAttributes.EnvironmentId),
+					zap.Int("attributeCount", len(userAttributes.UserAttributes)),


Suggested change

zap.String("environmentId", userAttributes.EnvironmentId),

zap.Int("attributeCount", len(userAttributes.UserAttributes)),

zap.String("environmentId", userAttributes.EnvironmentId),

zap.Any("attributes", userAttributes.UserAttributes),

zap.Int("attributeCount", len(userAttributes.UserAttributes)),

kakcy

@cre8ivejp
Thank you for your review.
The review pointed out have been fixed.

Also, as we discussed, we don't need to save all requests in Redis because 90% of the cases would have the same user attributes. This will increase the cost of the Redis Private service connection.
Please check the way we did it to save the last_used_info logic.

I have modified it so that it caches in-memory based on the last_used_info logic, and then caches asynchronously in Redis periodically.

Please check it out when you have free time.

kakcy · 2025-07-17T01:46:00Z

 	}
 }

+func TestGetUserAttributeKeys(t *testing.T) {


I Added e2e test.

cre8ivejp · 2025-07-17T05:32:54Z

+			for _, attr := range userAttributesMap {
+				userAttributes.UserAttributes = append(userAttributes.UserAttributes, attr)
+			}
+			p.userAttributesCache[environmentId] = userAttributes


The current implementation overrides the current cache with every request.
If the event is from iOS with 4 attributes and the next one is from Android with 2 attributes, the iOS attributes will not be saved.

cre8ivejp · 2025-07-17T05:36:31Z

+			Id:   userID,
+			Data: data,
+		},
+		Reason: &featureproto.Reason{},


Set the reason to CLIENT to avoid errors in the subscriber.

cre8ivejp · 2025-07-17T05:37:28Z

+
+	testUserDataKeySuffix := "testGetUserAttributeKeys-"
+	data := map[string]string{
+		testUserDataKeySuffix + uuid: "0.1.0",


@add at least 3 attributes, so you ensure that the loop saving the keys in the subscriber is working properly.

cre8ivejp · 2025-07-17T05:39:52Z

+		testUserDataKeySuffix + uuid: "0.1.0",
+	}
+
+	evaluation, err := ptypes.MarshalAny(&eventproto.EvaluationEvent{


Send at least 2 different Evaluation events using different source IDs and attributes. So, we ensure that is saving it correctly without overriding the attributes from other events.

cre8ivejp · 2025-07-17T05:41:38Z

+		FeatureId:      featureID,
+		FeatureVersion: 1,
+		UserId:         userID,
+		VariationId:    "variation-id",


Use the get feature API to retrieve the flag you created for testing, and get the correct variation ID and feature version, to avoid errors and buggy states when saving in the subscriber.

cre8ivejp · 2025-07-22T04:10:43Z

+func (p *evaluationCountEventPersister) writeUserAttributes() {
+	p.userAttributesCacheMutex.Lock()
+	defer p.userAttributesCacheMutex.Unlock()
+
+	for _, cache := range p.userAttributesCache {
+		if cache != nil && len(cache.UserAttributes) > 0 {
+			if err := p.upsertUserAttributes(cache); err != nil {
+				continue
+			}
+		}
+	}
+	// Reset the cache
+	p.userAttributesCache = make(userAttributesCache)
+}


WDYT of the following approach?
If Redis is down or being restarted, if an error happens, it would delete all in-memory cache.
The example below would only delete the data if it succeeds.

Suggested change

func (p *evaluationCountEventPersister) writeUserAttributes() {

p.userAttributesCacheMutex.Lock()

defer p.userAttributesCacheMutex.Unlock()

for _, cache := range p.userAttributesCache {

if cache != nil && len(cache.UserAttributes) > 0 {

if err := p.upsertUserAttributes(cache); err != nil {

continue

}

}

}

// Reset the cache

p.userAttributesCache = make(userAttributesCache)

}

func (p *evaluationCountEventPersister) writeUserAttributes() {

p.userAttributesCacheMutex.Lock()

defer p.userAttributesCacheMutex.Unlock()

for envID, cache := range p.userAttributesCache {

if cache != nil && len(cache.UserAttributes) > 0 {

if err := p.upsertUserAttributes(cache); err != nil {

p.logger.Error(

"Failed to save user attributes, will retry next cycle",

zap.Error(err),

zap.String("environmentId", envID),

)

continue

}

// If successful, delete it from the cache.

// The failed items will remain for the next attempt.

delete(p.userAttributesCache, envID)

}

}

}

kakcy · 2025-07-22T09:28:33Z

@cre8ivejp
Thank you!
Your suggestion has been accepted. Please check it out when you have time.
e459365

cre8ivejp

Nice work!
Thank you 🎉

kakcy added 2 commits July 14, 2025 18:44

feat: add GetUserAttributeKeys

745a7e6

reformat

f6dab2f

kakcy requested a review from Copilot July 15, 2025 03:27

Copilot AI reviewed Jul 15, 2025

View reviewed changes

reformat

c7f6326

kakcy marked this pull request as ready for review July 15, 2025 05:40

kakcy requested review from cre8ivejp, hvn2k1 and nnnkkk7 as code owners July 15, 2025 05:40

cre8ivejp reviewed Jul 15, 2025

View reviewed changes

kakcy added 3 commits July 16, 2025 20:24

Changes to save User Attribute Key asynchronously.

f34edc3

reformat

f6f9b1f

fix test

2b57793

kakcy commented Jul 17, 2025

View reviewed changes

cre8ivejp reviewed Jul 17, 2025

View reviewed changes

kakcy added 2 commits July 18, 2025 14:01

UserAttribute saving now works on multiple events

c2e8525

fix TestGetUserAttributeKeys for e2e

d3e0ad3

cre8ivejp reviewed Jul 18, 2025

View reviewed changes

Comment thread pkg/subscriber/processor/evaluation_events_evaluation_count_event_persister.go

cre8ivejp reviewed Jul 22, 2025

View reviewed changes

kakcy added 2 commits July 22, 2025 14:41

fix to leave in-memory cache when writing to Redis fails

e459365

fix flaky tests

f2b668c

cre8ivejp approved these changes Jul 23, 2025

View reviewed changes

kakcy merged commit 6b46e84 into main Jul 23, 2025

kakcy deleted the add-GetUserAttributeKeys branch July 23, 2025 06:15

bucketeer-bot mentioned this pull request Jul 23, 2025

chore(main): release 2.0.0 #1548

Merged

cre8ivejp mentioned this pull request Aug 8, 2025

feat: generate automatically the custom user attribute list in the targeting rules #686

Closed

	// Update the feature flag last-used cache
	p.cacheLastUsedInfoPerEnv(envEvents)
	updateEvaluationCounter(envEvents)
	case <-ticker.C:
	envEvents := p.extractEvents(batch)
	// Update the feature flag last-used cache
	p.cacheLastUsedInfoPerEnv(envEvents)

		zap.String("environmentId", userAttributes.EnvironmentId),
		zap.Int("attributeCount", len(userAttributes.UserAttributes)),

Uh oh!

Conversation

kakcy commented Jul 15, 2025

API Enhancements:

Caching and Service Updates:

Logic for User Attributes Processing:

Testing:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kakcy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kakcy commented Jul 22, 2025

Uh oh!

cre8ivejp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants