feat: [log retention improvements pt. 3] impl and use per hour file layout #2570

tedim52 · 2024-10-17T10:15:08Z

Description

Log Retention Improvements:

PR 1: Introduce LogFileLayout and PerWeekLogFileLayout and adjust LogFileManager and LogsDatabaseClient to use this for retrieving log file paths
PR 2: Make retention configurable via a CLI flag
PR 3: Implement PerHourFileLayout and swap storage format from PerWeekFileLayout to PerHourFileLayout
PR 4: Make log retention configurable via kurtosis config

Is this change user facing?

YES

References

#2536

… todos

tedim52 · 2024-10-21T08:09:44Z

...entralized_logs/client_implementations/persistent_volume/file_layout/per_hour_file_layout.go

+	baseLogsFilePath string
+}
+
+func NewPerHourFileLayout(time logs_clock.LogsClock, baseLogsFilePath string) *PerHourFileLayout {


This implementation of FileLayout stores log files per hour so that logs can be removed per hour.

Can we put this in a code comment so it doesn't get lost when this PR is closed?

...ent_implementations/persistent_volume/stream_logs_strategy/stream_logs_strategy_impl_test.go

tedim52 · 2024-10-21T08:15:16Z

engine/server/engine/main.go

-		logsDatabaseClient = persistent_volume.NewPersistentVolumeLogsDatabaseClient(kurtosisBackend, osFs, logFileManager, perWeekStreamLogsStrategy)
+		perHourFileLayout := file_layout.NewPerHourFileLayout(realTime, volume_consts.LogsStorageDirpath)
+		logFileManager := log_file_manager.NewLogFileManager(kurtosisBackend, osFs, perHourFileLayout, realTime, logRetentionPeriod, volume_consts.LogsStorageDirpath)
+		if logFileManager.GetLogFileLayoutFormat() != vector_consts.VectorLogsFilepathFormat {


This check ensures that the log file format thats being used to read log files matches the format the logs aggregator is writing log files - if anyone changes the write format or the read format without updating the other, this should fail.

Nice context! Can this be a comment to capture that this is an extra sanity check?

internal_testsuites/golang/testsuite/startlark_user_passing_test/starlark_user_passing_test.go

tedim52 · 2024-10-22T15:32:09Z

...entralized_logs/client_implementations/persistent_volume/file_layout/per_hour_file_layout.go

+	var paths []string
+	retentionPeriodInHours := DurationToHours(retentionPeriod)
+
+	if retentionPeriodIntervals < 0 {


maybe could find a way to merge this getLogFilePathsFromNowTillRetentionPeriod and getLogFilePathsBeyondRetentionPeriod?

That seems sensible to me!

mieubrisse

Made it through!

mieubrisse · 2024-11-02T21:19:36Z

...ls/docker/docker_kurtosis_backend/logs_aggregator_functions/implementations/vector/consts.go

 	// To construct the filepath, we utilize vectors template syntax that allows us to reference fields in log events
 	// https://vector.dev/docs/reference/configuration/template-syntax/
-	baseLogsFilepath = "\"" + logsStorageDirpath + "%%Y/%%V/"
+	baseLogsFilepath = "\"" + logsStorageDirpath + "%%Y/%%V/%%u/%%H/"


Might be worth a comment explaining what this does, equivalent to the one removed

mieubrisse · 2024-11-02T21:22:11Z

...entralized_logs/client_implementations/persistent_volume/file_layout/per_hour_file_layout.go

+	perHourFilePathFmtSt = perHourDirPathFmtStr + "%s/%s%s"
+)
+
+type PerHourFileLayout struct {


What does this struct actually represent?

mieubrisse · 2024-11-02T21:24:24Z

...entralized_logs/client_implementations/persistent_volume/file_layout/per_hour_file_layout.go

+	}
+}
+
+func (phf *PerHourFileLayout) GetLogFileLayoutFormat() string {


Given that this seems to be very vector-specific, should this be called GetVectorLayoutFormat or something like that?

mieubrisse · 2024-11-02T21:25:07Z

...entralized_logs/client_implementations/persistent_volume/file_layout/per_hour_file_layout.go

+func (phf *PerHourFileLayout) GetLogFileLayoutFormat() string {
+	// Right now this format is specifically made for Vector Logs Aggregators format
+	// This wil be used my Vector LogsAggregator to determine the path to output to
+	return fmt.Sprintf("\"%s%%%%Y/%%%%V/%%%%u/%%%%H/{{ enclave_uuid }}/{{ service_uuid }}.json\"", volume_consts.LogsStorageDirpath)


in this case, rather than wrestling with all the crazy escaping, might be cleaner to read with a simple " + volume_consts.LogsStorageDirpath + ...the rest...

mieubrisse · 2024-11-02T21:25:54Z

...entralized_logs/client_implementations/persistent_volume/file_layout/per_hour_file_layout.go

+}
+
+func (phf *PerHourFileLayout) GetLogFilePath(time time.Time, enclaveUuid, serviceUuid string) string {
+	year, week, day, hour := TimeToWeekDayHour(time)


Tiny little sixth sense wondering why this is a public function, and if it should be private first (but will wait to see what happens in the rest of the PR)

mieubrisse · 2024-11-14T17:45:55Z

...mplementations/persistent_volume/persistent_volume_helpers/persistent_volume_helpers_test.go

+	"time"
+)
+
+func TestConvertWeeksToDuration(t *testing.T) {


I love how methodical you are with testing 😍

mieubrisse · 2024-11-14T17:51:11Z

engine/server/engine/main.go

-		logsDatabaseClient = persistent_volume.NewPersistentVolumeLogsDatabaseClient(kurtosisBackend, osFs, logFileManager, perWeekStreamLogsStrategy)
+		perHourFileLayout := file_layout.NewPerHourFileLayout(realTime, volume_consts.LogsStorageDirpath)
+		logFileManager := log_file_manager.NewLogFileManager(kurtosisBackend, osFs, perHourFileLayout, realTime, logRetentionPeriod, volume_consts.LogsStorageDirpath)
+		if logFileManager.GetLogFileLayoutFormat() != vector_consts.VectorLogsFilepathFormat {


Nice context! Can this be a comment to capture that this is an extra sanity check?

mieubrisse · 2024-11-14T17:54:12Z

...s/client_implementations/persistent_volume/stream_logs_strategy/stream_logs_strategy_impl.go

 )

-// PerWeekStreamLogsStrategy pulls logs from filesystem where there is a log file per year, per week, per enclave, per service
+// StreamLogsStrategyImpl pulls logs from filesystem where there is a log file per year, per week, per enclave, per service


Is this comment still true? From what I understand, it's now transparent to whether it's per-week or per-hour right?

mieubrisse · 2024-11-14T17:58:02Z

...s/client_implementations/persistent_volume/stream_logs_strategy/stream_logs_strategy_impl.go

-	logRetentionPeriodInWeeks int
+type StreamLogsStrategyImpl struct {
+	time               logs_clock.LogsClock
+	logRetentionPeriod time.Duration


I'm trying to figure out why this struct cares about the retention period and fileLayout. From what I understood above, it's the LogManager that's responsible for retention, and interfacing with the underlying fileLayout, so wouldn't that mean that StreamLogsStrategy should interface with LogsManager instead?

Otherwise, isn't StreamLogsStrategy then going to have to reimplement logic for retention-y stuff? (Or, maybe this is why FileLayout has the retention-y stuff in it, that confused me earlier..? 🤔)

mieubrisse · 2024-11-14T18:00:28Z

...logs/client_implementations/persistent_volume/persistent_volume_logs_database_client_test.go

@@ -154,6 +157,56 @@ func TestStreamUserServiceLogsPerWeek_WithFilters(t *testing.T) {
 	require.NoError(t, testEvaluationErr)
 }

+func TestStreamUserServiceLogsPerHour_WithFilters(t *testing.T) {


Again, nice job with these tests! I know that testing something as complex as this can feel like a drag, but it makes the code sooo much safer 👍

tedim52 added 30 commits July 31, 2024 09:00

add time measurements in cli, engine, log file parsing

ee510cd

add more granular measurements

f881126

use buffered log channel

487e5d1

batch send log lines

f316abe

refactor to use log line sender

0614976

encapsulate buff channel inside log line sender

75fd409

refactor again and get tests to pass

42f7a30

flush logs and close channel when empty

5fe30ff

clean up

929f4b2

undo build script change

fdd8bf3

Merge branch 'main' into tedi/logspeedup

0883f39

name mutex

dc9d1d1

lint

d2b9f86

increase seconds to wait for logs

813c98b

rename send logl ine

6349c61

move log line before function

9510e22

flush before follow

3b73af1

clear buffers after flushing

470c61f

revert times

2c489e2

lint

2c1f0bf

turn off cypress tests

815eded

remove k cloud ref

ccd49c3

use latest docs checker

684cbd9

use latest docs checker again

fff5ff0

create file layout interface

d00bb2b

reimplement per week using file layout

8734578

progress on file layout

3febce2

add get log filepath, migrate tests to use get log filepath, add some…

7d3dd38

… todos

Merge branch 'main' into tedi/granularetention

11449c1

remove screenshots

aacffb6

tedim52 added 14 commits October 17, 2024 19:22

revert go work to go1.2

b1d5ef2

revert go mods

646fa15

convert to duration

6234fc2

fix some tests

936b4ad

rename per week stream logs strategy

6fb7218

update go sums

6edc128

lint engine

84a31f2

update vector storage path

d0061b0

add todo

a5f7c46

change to lowercase u

b4124e2

parametrize base logs storage path

3cb70fa

unformat day

0c4d4ea

convert sunday from golang time to vector time

e724d90

add check to ensure formats are equivalent

1d6ff1e

tedim52 force-pushed the tedi/perhour branch from 2a23d24 to 1d6ff1e Compare October 18, 2024 08:30

tedim52 added 3 commits October 18, 2024 17:46

fix filepath formatting and check

9102416

adjust log stmt

b8ce98e

pin besu img to pass tests

304ade0

tedim52 commented Oct 21, 2024

View reviewed changes

...ent_implementations/persistent_volume/stream_logs_strategy/stream_logs_strategy_impl_test.go Show resolved Hide resolved

tedim52 commented Oct 21, 2024

View reviewed changes

internal_testsuites/golang/testsuite/startlark_user_passing_test/starlark_user_passing_test.go Show resolved Hide resolved

tedim52 added 4 commits October 21, 2024 18:31

refactor and test convert weeks to duration

642e7f3

fix newline in go work

24ecb4a

update persistent volume db tests

5c32a08

add across week day hour test

8f83284

tedim52 commented Oct 22, 2024

View reviewed changes

simplify get all log filepaths and add cmt

85012df

tedim52 requested a review from mieubrisse October 22, 2024 15:47

mieubrisse approved these changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [log retention improvements pt. 3] impl and use per hour file layout #2570

feat: [log retention improvements pt. 3] impl and use per hour file layout #2570

tedim52 commented Oct 17, 2024 •

edited

Loading

tedim52 Oct 21, 2024

mieubrisse Nov 2, 2024

tedim52 Oct 21, 2024 •

edited

Loading

mieubrisse Nov 14, 2024

tedim52 Oct 22, 2024

mieubrisse Nov 2, 2024

mieubrisse left a comment

mieubrisse Nov 2, 2024

mieubrisse Nov 2, 2024

mieubrisse Nov 2, 2024

mieubrisse Nov 2, 2024

mieubrisse Nov 2, 2024

mieubrisse Nov 14, 2024

mieubrisse Nov 14, 2024

mieubrisse Nov 14, 2024

mieubrisse Nov 14, 2024

mieubrisse Nov 14, 2024

feat: [log retention improvements pt. 3] impl and use per hour file layout #2570

Are you sure you want to change the base?

feat: [log retention improvements pt. 3] impl and use per hour file layout #2570

Conversation

tedim52 commented Oct 17, 2024 • edited Loading

Description

Is this change user facing?

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tedim52 Oct 21, 2024 • 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

mieubrisse 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

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

tedim52 commented Oct 17, 2024 •

edited

Loading

tedim52 Oct 21, 2024 •

edited

Loading