Skip to content

Change implementation of user activity records split to match resource presence#35553

Closed
Envek wants to merge 3 commits intogravitational:masterfrom
Envek:envek/user-activity-reporting-refactor
Closed

Change implementation of user activity records split to match resource presence#35553
Envek wants to merge 3 commits intogravitational:masterfrom
Envek:envek/user-activity-reporting-refactor

Conversation

@Envek
Copy link
Copy Markdown
Contributor

@Envek Envek commented Dec 8, 2023

@github-actions github-actions Bot requested review from klizhentas and zmb3 December 8, 2023 12:19
@zmb3 zmb3 requested a review from espadolini December 8, 2023 23:19
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Dec 8, 2023

Looks fine to me but lets have @espadolini review next week.

}

report.Records = report.Records[:len(report.Records)/2]
records = recordsTail
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of doing it like this? It avoids moving data around and it makes it more obvious that we're just repeatedly splitting all the original records slice into reports.

Suggested change
records = recordsTail
records = records[len(report.Records):]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks! Applied your suggestion, also added test to check that no records lost

Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>
Comment thread lib/usagereporter/teleport/aggregating/service_test.go Outdated
Co-authored-by: Zac Bergquist <zbergquist99@gmail.com>
@zmb3 zmb3 changed the title Change implementation of user activity records split to match #34954 Change implementation of user activity records split to match resource presence Jan 10, 2024
@zmb3 zmb3 closed this in #36501 Jan 10, 2024
@Envek Envek deleted the envek/user-activity-reporting-refactor branch January 15, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants